The fact SessionMaker.close_all closes all sessions even when called on an instance is confusing

[autra]

Description

When calling SessionMaker.close_all, it closes all the opened sessions by sqlalchemy, which can be error-prone and confusing.

use case

I'm developing a project using a lib called pywps. Pywps has its own SessionMaker instance (in memory db) and my apps has a postgresql db with its own SessionMaker too.

When pywps calls SessionMaker.close_all() here, all my sessions are also closed at the same time.

While pywps should probably not do that (close_all is a classmethod after all, and the documentation does say "close all session in memory"), I still find this fact confusing for users. Intuitively I would have expected this call to close only the Session associated with this SessionMaker.

Suggestion

Maybe close_all on a session_maker instance should only close sessions made by this instance ?

Thanks!

[zzzeek]

I would welcome a contribution that makes this a standalone function and applies the appropriate deprecation decorators to the existing method.

[zzzeek]

deprecation decorators: https://github.com/sqlalchemy/sqlalchemy/blob/master/lib/sqlalchemy/util/deprecations.py#L25

new close_all function would be in the same level as: https://github.com/sqlalchemy/sqlalchemy/blob/master/lib/sqlalchemy/orm/session.py#L3150

[autra]

@zzzeek thanks for your answer.

new close_all function would be in the same level as: /lib/sqlalchemy/orm/session.py@master#L3150

Same level as - for instance - make_transient (line numbers seems to have changed in current master), am I right?

It may be useful to also add an instance method on SessionMaker that would close only the sessions of this SessionMaker? The deprecated message on the class method would then mention both methods. What do you think?

[zzzeek]

@zzzeek thanks for your answer.

new close_all function would be in the same level as: /lib/sqlalchemy/orm/session.py@master#L3150

Same level as - for instance - make_transient (line numbers seems to have changed in current master), am I right?

yes, e.g. not a method, a module level function

It may be useful to also add an instance method on SessionMaker that would close only the sessions of this SessionMaker?

that would be quite complicated to add and I don't think there's really much of a use case for "close_all" overall, plus the long term future of "sessionmaker" itself is somewhat doubtful.

The deprecated message on the class method would then mention both methods. What do you think?

"close all" IMO is a so-called "utility" use case, that is, not a normal thing you should be doing, it's more like when you are writing teardown schemes in test suites where you need to do blunt things so I think just a module-level function is sufficient. if OTOH you really have a real use case for sessionmaker.close_all() I'd love to hear it.

[autra]

@zzzeek so I have a branch ready, but I'm not sure what test I can add to it (the PR template makes it quite clear I need one!). Do you have a suggestion?

[zzzeek]

@zzzeek so I have a branch ready, but I'm not sure what test I can add to it (the PR template makes it quite clear I need one!). Do you have a suggestion?

ha, that PR template is inspired more by the unsolicited "drive by" PRs I keep getting, this PR is at least agreed upon and sanctioned.

I took a look to see if we have any close_all tests:

$ grep -H "close_all" `find test -name "*.py"`
test/ext/declarative/test_inheritance.py:        Session.close_all()
test/ext/declarative/test_mixin.py:        Session.close_all()
test/ext/declarative/test_basic.py:        Session.close_all()
test/orm/test_eager_relations.py:        session.close_all()
test/orm/test_subquery_relations.py:        session.close_all()

based on the names of those tests I can say that's fixture teardown code, not an actual test of close_all() itself. So general session-ish tests go into test/orm/test_session.py, and util-ish things like close_all can be in SessionUtilTest: https://github.com/sqlalchemy/sqlalchemy/blob/master/test/orm/test_session.py#L148 . session.close() is also weird in that the session doesn't go into a "closed" state, so the name is not even that great, it should be something like .reset() someday, but not today.

here's a test that works there, which would be modified to use the new function:

    def test_close_all(self):
        users, User = self.tables.users, self.classes.User

        mapper(User, users)

        s1 = Session()
        u1 = User()
        s1.add(u1)

        assert u1 in s1

        Session.close_all()

        assert u1 not in s1

also for the deprecation warning, there's a decorator you should use which you can see used here: https://github.com/sqlalchemy/sqlalchemy/blob/master/lib/sqlalchemy/orm/session.py#L1758

when you put that decorator on the close_all() classmethod, the other tests that are calling it in their teardown are going to break, because we fail if a warning occurs. so the other files that are using Session.close_all() normally need to be modified to use the new function.

Here's how to run the tests in a fairly quick way (most PRs I get, even when they have tests, the tests break, meaning they never ran the tests):

    # run just your new test
    py.test test/orm/test_session.py -k test_close_all

   # run all the ORM tests, -n4 will let it run faster, needs pytest-xdist installed
   py.test -n4 test/orm/

[autra]

Thanks for your input, and the quickstart tips!

if OTOH you really have a real use case for sessionmaker.close_all() I'd love to hear it.

Yeah, no, not really actually. Let's forget it.

[autra]

I mean, the way pywps uses it here does seem very strange to me. Unless I missed something, it should probably not be called at all.

[sqla-tester]

Augustin Trancart has proposed a fix for this issue in the master branch:

Add standalone orm.close_all method and deprecate SessionMaker.close_all https://gerrit.sqlalchemy.org/1090

[sqla-tester]

Augustin Trancart has proposed a fix for this issue in the master branch:

Add standalone orm.close_all method and deprecate SessionMaker.close_all https://gerrit.sqlalchemy.org/1090