Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alembic doesn't cleaup up after itself #25

Closed
sqlalchemy-bot opened this issue Jan 25, 2012 · 8 comments
Closed

Alembic doesn't cleaup up after itself #25

sqlalchemy-bot opened this issue Jan 25, 2012 · 8 comments
Labels
bug
Milestone

Comments

@sqlalchemy-bot
Copy link

@sqlalchemy-bot sqlalchemy-bot commented Jan 25, 2012

Migrated issue, originally created by Daniel Miller (@millerdev)

Some alembic commands (for example, upgrade and downgrade) create an engine and a connection, but these resources are not cleaned up on exit. I have some tests that create a new database, run alembic commands, and finally drop the database. However, the database cannot be dropped until the alembic resources are cleaned up. These resources are created in env.py. Should the env.py templates include some resource cleanup?

#!python

engine = engine_from_config(
    config.get_section(config.config_ini_section),
    prefix='sqlalchemy.')

connection = engine.connect()
try:
    context.configure(
                connection=connection, 
                target_metadata=target_metadata
                )

    with context.begin_transaction():
        context.run_migrations()
finally:
    connection.close()
    engine.dispose()
@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Jan 25, 2012

Michael Bayer (@zzzeek) wrote:

I really would prefer if a "with:" could be used here, but as Connection doesn't seem to have that interface right now, there can be a close() at the end. The dispose() I'm much less excited about. People really get the wrong idea about dispose() just by seeing that it exists. It isn't needed in any way for your tables to be dropped, just the close() so that transactional locks are released.

If you're embedding into a test here, something I do on this end as well, your env.py would best be modified to get at the engine that's being run for the tests.

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Jan 25, 2012

Changes by Michael Bayer (@zzzeek):

  • set milestone to "0.2.0"
@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Jan 26, 2012

Daniel Miller (@millerdev) wrote:

with connection:}}} would be great.
{{{with connection:

With regard to engine.dispose(), you seem to be suggesting that its not necessary. However, when I omit it my tests fail because there are still active connections in the pool and I can't drop the database. What I'd really like is engine.close(), which would close all active (and not detached) connections and invalidate the engine so no new connections can be made, like how the DBAPI's connection.close() invalidates all cursors that are using the connection.

It's not the test's job to dig into the things that are being tested to free up resources.

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Jan 26, 2012

Michael Bayer (@zzzeek) wrote:

My thinking at the moment on this is that it's not really like connection.close() closing out cursors as much as a function like DBAPI->close_all_connections(), which they of course don't have because it's redundant. They'd tell you to call close() on each connection.

If you want to track all connections, you can do a "connect" event and put all the connections into a WeakKeyDictionary as they are intercepted by the event. The close() then closes out whatever is in that dictionary. The SQLA test suite does it, and it was extremely difficult to make it work in all cases; I'm not sure if it still doesn't miss some edge cases.

So my reservations building this in are a. it dilutes the simple model of "close your connections", b. it's very hard to make it work 100% and c. it promotes even more confusion about the role of the engine.

I'm shocked how often I see tutorials, mailing list emails, doing this:

#!python

def query_the_database():
    """Query with SQLAlchemy.  SQLAlchemy requires that we create an 
    engine, *and* connect in order to get to the database"
    engine = create_engine(...)
    connection = engine.connect()
    results = connection.execute("...")
    connection.close()
    engine.dispose()
    return results

It's news to me that PG doesn't let you DROP DATABASE if someone is just connected to it, but this seems to be the case.

Anyway this is more a sqlalchemy feature request, for the task at hand i added the close() as well as just used NullPool which I think is more appropriate here, curious users will learn more that way than if they saw a dispose(), that's in [[https://bitbucket.org/zzzeek/alembic/changeset/279f8359031a363e0c43a49dd87ac133eba4f6e9|279f8359031a363e0c43a49dd87ac133eba4f6e9]]. reopen if that doesn't work for you, thanks !

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Jan 26, 2012

Changes by Michael Bayer (@zzzeek):

  • changed status to closed
@sqlalchemy-bot sqlalchemy-bot added the bug label Nov 27, 2018
@sqlalchemy-bot sqlalchemy-bot added this to the 0.2.0 milestone Nov 27, 2018
@hartzell
Copy link

@hartzell hartzell commented May 29, 2020

I'm finding myself in a similar situation, where I need to call engine.dispose() in my application code so that my tests are able to drop the database and succeed.

I just want my application to be able to check the schema version, and put this together (cribbed from here).

def get_actual_schema_revision(uri):
    current_rev = None
    try:
        engine = create_engine(uri)
        conn = engine.connect()
        context = MigrationContext.configure(conn)
        current_rev = context.get_current_revision()
    finally:
        conn.close()
        engine.dispose()

    return current_rev

The changeset you cite above is no longer available. I can see the diff between those two commits by cloning the repo and running:

git diff 279f8359031a363e0c43a49dd87ac133eba4f6e9^..279f8359031a363e0c43a49dd87ac133eba4f6e9

but I guess I'm in the 'curious user' class you mention. It seems uglier for me to iterate over all the connections.

If we're not supposed to call engine.dispose, then what is the proper way to clean something like this up?

@zzzeek
Copy link
Member

@zzzeek zzzeek commented May 29, 2020

I'm finding myself in a similar situation, where I need to call engine.dispose() in my application code so that my tests are able to drop the database and succeed.

this should never be necessary. ensure you commit or rollback all transactions in progress before ending your test suite.

I just want my application to be able to check the schema version, and put this together (cribbed from here).

def get_actual_schema_revision(uri):
    current_rev = None
    try:
        engine = create_engine(uri)
        conn = engine.connect()
        context = MigrationContext.configure(conn)
        current_rev = context.get_current_revision()
    finally:
        conn.close()
        engine.dispose()

conn.close() is enough as that will roll back any transactional state on the connection that's holding locks.

The changeset you cite above is no longer available. I can see the diff between those two commits by cloning the repo and running:

this is an ancient issue. the codebase has changed completely since it was open and has been migrated from mercurial to git years ago.

If we're not supposed to call engine.dispose, then what is the proper way to clean something like this up?

connection.close(). if that's not working then please open a new issue with a complete MCVE and please include complete information about database, DBAPI / driver version in use. thanks!

@hartzell
Copy link

@hartzell hartzell commented May 29, 2020

if that's not working then please open a new issue with a complete MCVE and please include complete information about database, DBAPI / driver version in use.

Done, see #702.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants