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

sqlalchemy session closing during use of plugin #1

Closed
mitechie opened this Issue Apr 19, 2011 · 9 comments

Comments

Projects
None yet
2 participants
@mitechie

We're using the SA plugin for .who and .what in our Pylons application. We were hitting a bug where a site with little activity would continuously get the "mysql server has gone away" error.

After some debugging, we found that in certain cases (e.g. a successful logout) a sqlalchemy Session would be used, but never closed. In the case of a pylons app there app wraps the call around a try/finally where the finally does a Session.remove() call. If the repoze.who middleware determined things were good and performed a redirect, the Pylons app would be skipped and the remove() wasn't called.

I can't seem to find a good place to put this kind of cleanup code in the plugins. What I neded up doing, to test if this was correct, was to import my DB session into repoze.who.middleware and right before it returns, to perform my DB cleanup:

    logger and logger.info(_ENDED % path_info)

    # import our Pylons app db session so we can remove/return it to the sqlalchemy pool
    from websim_app.model import meta
    meta.Session.remove()

    return app_iter

This was brought up in this thread here:
https://groups.google.com/forum/#!topic/pylons-discuss/DA8f4VyEEwM/discussion

I'm trying to figure out how to manage this long term. I hate manually import db code into the repoze.who.middleware, but it was the only place I could see that would always be run. Has this come up before? Is there a better practice for dealing with this?

Thanks for any assistance.

@gnarea

This comment has been minimized.

Show comment
Hide comment
@gnarea

gnarea Apr 19, 2011

Member

Hello, mitechie!

Thanks for bringing this up.

Please have a look at this: https://groups.google.com/d/msg/pylons-discuss/DA8f4VyEEwM/XWO6opVgCwMJ

Cheers.

Member

gnarea commented Apr 19, 2011

Hello, mitechie!

Thanks for bringing this up.

Please have a look at this: https://groups.google.com/d/msg/pylons-discuss/DA8f4VyEEwM/XWO6opVgCwMJ

Cheers.

@mitechie

This comment has been minimized.

Show comment
Hide comment
@mitechie

mitechie Apr 20, 2011

Thanks! We're testing the updated package on the application today. If it works in the short test we'll run the long term tests with the normal 8hr mysql timeouts and such. It might take a couple of days to 100% verify we no longer get the issue. I'll let you know how it goes.

Thanks! We're testing the updated package on the application today. If it works in the short test we'll run the long term tests with the normal 8hr mysql timeouts and such. It might take a couple of days to 100% verify we no longer get the issue. I'll let you know how it goes.

@gnarea

This comment has been minimized.

Show comment
Hide comment
@gnarea

gnarea Apr 20, 2011

Member

That's excellent, thank you! I'll release it as soon as you confirm it's fixed (unless it's not :) ).

Member

gnarea commented Apr 20, 2011

That's excellent, thank you! I'll release it as soon as you confirm it's fixed (unless it's not :) ).

@mitechie

This comment has been minimized.

Show comment
Hide comment
@mitechie

mitechie Apr 25, 2011

We're seeing some errors still. We've got a test app that is being used and it's showing the errors with the latest git pull of repoze.who-sqlalchemy. If I go back and add the session.remove into the repoze.who.middleware it goes away. I'm working on getting the test case together and will see if I can trace/find a suggestion from here, but just a heads up.

We're seeing some errors still. We've got a test app that is being used and it's showing the errors with the latest git pull of repoze.who-sqlalchemy. If I go back and add the session.remove into the repoze.who.middleware it goes away. I'm working on getting the test case together and will see if I can trace/find a suggestion from here, but just a heads up.

@gnarea

This comment has been minimized.

Show comment
Hide comment
@gnarea

gnarea Apr 27, 2011

Member

Hello, mitechie! Thanks for the heads up. Have you been able to reproduce it? Cheers.

Member

gnarea commented Apr 27, 2011

Hello, mitechie! Thanks for the heads up. Have you been able to reproduce it? Cheers.

@mitechie

This comment has been minimized.

Show comment
Hide comment
@mitechie

mitechie Apr 27, 2011

Sorry for the delay. We updated the live sites and they were good for 24 hrs when we hit an issue with the repoze.who authtkt cookie plugin not using the actual url path of the apps to handle cookies (it just hard codes to / ). So we had to make code changes and restart all of the apps running today. The 7 are now back up and running and we're starting the clock over.

In my local test environment it seems corrected and your fix makes sense to me, so I think we're good. However, the boss wants to see things stay up before calling this one fixed.

Sorry for the delay. We updated the live sites and they were good for 24 hrs when we hit an issue with the repoze.who authtkt cookie plugin not using the actual url path of the apps to handle cookies (it just hard codes to / ). So we had to make code changes and restart all of the apps running today. The 7 are now back up and running and we're starting the clock over.

In my local test environment it seems corrected and your fix makes sense to me, so I think we're good. However, the boss wants to see things stay up before calling this one fixed.

@gnarea

This comment has been minimized.

Show comment
Hide comment
@gnarea

gnarea Apr 27, 2011

Member

Yeah, it's usually best to wait with this sort of things, that's why I haven't released the fix yet (i.e., to avoid an epic fail if it's not really fixed :) ). Thank you!

Member

gnarea commented Apr 27, 2011

Yeah, it's usually best to wait with this sort of things, that's why I haven't released the fix yet (i.e., to avoid an epic fail if it's not really fixed :) ). Thank you!

@mitechie

This comment has been minimized.

Show comment
Hide comment
@mitechie

mitechie Apr 29, 2011

So we're 2days no errors on any of the applications running which I'm told is a new world record. I'm going to call it good I think. Thanks so much for the assistance in this, can't say enough how awesome open source is like this.

So we're 2days no errors on any of the applications running which I'm told is a new world record. I'm going to call it good I think. Thanks so much for the assistance in this, can't say enough how awesome open source is like this.

@gnarea

This comment has been minimized.

Show comment
Hide comment

@gnarea gnarea closed this May 2, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment