Update pool.py

[glyg]

Use if instead of a try / except loop to avoid circular call, see #827

[d-w-moore]

Seems fine. Can run a full test suite later today perhaps, which could be useful if I knew how to invoke #827. Not knowing how one might reproduce the original bug leaves me wondering how to write a good test.

[glyg]

I have trouble reproducing the bug.

Our setup is quite complex, and I manage to produce the bug, but not every time and not at the same time ...
With a populated DB, repeatedly querying irods through the python client leads to the error, but not on a given data object or after a specific number of repetitions.

I am trying to corner the bug better, but for now I can't give you a minimal example

[glyg]

Hi, we tracked it down to too many connection on the DB, which leads to the messy disconnect and death of the server.

This does not seem to happen on a pre-prod server, but only in the docker context above, on my machine.

[glyg]

further query to the DB fails

sql: error: connection to server on socket "/var/run/postgresql/.s.PGSQL.5432" failed: FATAL:  sorry, too many clients already

[trel]

ah, very good.

so, if we limit the resources artificially, we should be able to produce a test that presents the same behavior.

[glyg]

Yes, we're not seeing it in the prod postgresql VM,

we tracked the Postgresql connections (with : psql -U irods -d ICAT -c "SELECT count(*) FROM pg_stat_activity;" ) and they are 'regulated' in the VM and not in the docker container on my sandbox.

The "too many connections" error ends up killing the irods process though

We updated postgresql to 17 without changing the issue

[glyg]

so, if we limit the resources artificially, we should be able to produce a test that presents the same behavior

I suspect killing the db and trying a request from the python client is enough to reproduce the long traceback at the origin of the issue

[trel]

I suspect killing the db and trying a request from the python client is enough to reproduce the long traceback at the origin of the issue

oh, yes, that might do it. definitely not something we've tested before.

[glyg]

It seems that (again, docker only) the idle connections to the postgresql DB are not cleaned and clog the pile ... Who's at fault in the stack though ...

[glyg]

So, we appear to have fixed the problem with a strict reuse of the session for each 'agent', so:

class iRODSAgent:
    ...
    def __enter__(self):
        super().__enter__()
        self.session = irods_conn(self.conf)
        return self

    def __exit__(self, exc_type, exc_value, traceback):
        self.session.cleanup()
        super().__exit__(exc_type, exc_value, traceback)

What create zombie session is doing something like:

    # Within the agent class
    def do_smthg(self):
        session = iRODSSession(...)

        session.do_something()

and not

    # Within the agent class
    def do_smthg(self):

        self.session.do_something()

Fewww ^^'

[trel]

So I see two things that work?

• A code change that uses if/else rather than try/except
• Holding the current code differently

[d-w-moore]

@glyg, I have doubts as to whether I'm reproducing the same syndrome here, In my attempts I have duplicated similar errors, through a script such as this one:

import irods
import sys

sessions = [ ]

for x in range(110):
  ses=irods.helpers.make_session()
  sessions.append(ses)
  ses.collections.get(f'/tempZone/home/{ses.username}')

Presumablly because keeping the ses references in a list causes more client-to-iRODS-server sessions(aka connections) to stay around without a call to iRODSSession.cleanup than can be supported. On my test platform, however, the command you used (psql -U irods -d ICAT -c "SELECT count(*) FROM pg_stat_activity;) does not reveal accumulated DB connections, nor do I get "too many" type errors in my postgresql logs. It is true, though, that changing the iterations of the loop to say 90 (which is less than the number of default supportable DB connections) does not invoke the error, in my case.

Having said all this, I've tried with the code fixes provided in this PR and they don't seem to address the errors I was seeing. The "simple"/efficient way to handle that, for the above script anyway, is to move the creation of ses to a point preceding the loop, thereby reusing the same session object for each of the collection queries. It also runs much faster that way, not having to build up the iRODS connection and tear it down each time.

I'm not sure if your use-case involves hanging on to references of the old sessions or not. But apparently calling cleanup on the old objects eased your problems...

Whatever the case, I cannot seemingly reproduce this issue at all, let alone reliably, so I am not sure where that leaves us with implementing a test method validating the fix.

[d-w-moore]

All of that said, as stated there seems to be no disadvantage to us changing the try/except to if/then in that it would be more future-proof. I cannot see where the current additive risk of a spurious KeyError is arising, but that is beside the point, I think. I defer to others on whether the PR merges in the continued absence of a test....

[korydraughn]

After some discussion, @d-w-moore and I landed at the following.

• This PR does not fix the reported issue
  • We think the reported issue is due to how the PRC was being used
  • Reusing the existing session object resolved the problem, as reported by @glyg (see Update pool.py #828 (comment))
• This PR is a necessary change because it future-proofs the code
  • Consider the case where the code is modified and KeyError is raised after a successful call to .pop()

@d-w-moore is going to open a new issue which explains why this PR is necessary.

As for the issue opened by @glyg, I think that can be addressed by documenting which usage patterns lead to confusing behavior or poor performance.