Ticket #10625 (closed defect: fixed)

Opened 6 months ago

Last modified 5 months ago

AssertionError in trac.db.pool.PooledConnection.__del__

Reported by: anonymous Assigned to: hasienda
Priority: normal Component: AccountManagerPlugin
Severity: normal Keywords: database API
Cc: Trac Release: 1.0

Description (Last modified by hasienda)

On login and logout I get the following error (for each environment):

Exception exceptions.AssertionError: <exceptions.AssertionError instance at 0x89faeacc> in <bound method PooledConnection.__del__ of <trac.db.pool.PooledConnection object at 0xb47cc88c>> ignored

It is related with the single sign on feature. It can be easily fixed by explicitly closing the database connection with db.close after the commit in the following lines:

Attachments

Change History

11/16/12 08:03:04 changed by ms4py

Well, a cleaner fix would be to upgrade the database access in general.

11/16/12 08:03:15 changed by ms4py

  • cc set to ms4py.

(follow-up: ↓ 5 ) 11/16/12 09:05:36 changed by ms4py

I have upgraded the plugin to use the Trac 1.0 database API. Let me know if you are interested in the patch.

(in reply to: ↑ description ) 11/16/12 23:41:00 changed by hasienda

  • keywords set to database API.
  • description changed.

Replying to anonymous:

On login and logout I get the following error (for each environment):

Exception exceptions.AssertionError: ...

It is related with the single sign on feature. It can be easily fixed by explicitly closing the database connection with db.close after the commit in the following lines:

I strongly disagree, and this is backed by developer discussion. A forced close may yield other side-effects, and current Trac db API strongly advocates for not using such forceful actions to a transaction in general.

(in reply to: ↑ 3 ; follow-up: ↓ 7 ) 11/16/12 23:53:03 changed by hasienda

Replying to ms4py:

I have upgraded the plugin to use the Trac 1.0 database API. Let me know if you are interested in the patch.

I already suspected from last comment, that you meant specifically the Trac-1.0 db API.

Sure, this will happen, but keep in mind, that AccountManager is in rather widespread use. Maintaining backwards-compatibility, at least down to Trac-0.11, is a major development constraint for me. How could I do that with the new API? I know, that its adoption will yield significant improvements, but I'm sure I can't drop support for Trac-0.11 to 0.12.x anytime soon.

My plan is to fork 1.0 development later on, so your patch would give a jump-start and therefore is certainly welcome. But now it's much to early for that, especially 'no-way' before the upcoming acct_mgr-0.4 release, that I've prepared for months now.

11/17/12 00:00:32 changed by hasienda

I've been testing the SSO feature for stability over about 10 months in production. I had my Trac processes crashing on me even more nastily than reported here for month. But lately I got a grip even on the most random issues. I'll update the code accordingly, probably just not in a way that you expect by now.

(in reply to: ↑ 5 ) 11/17/12 11:04:32 changed by ms4py

Yes, it is obvious that an 1.0 targeting patch will require a new branch :) I will post the patch next week (don't have access to my Trac instance from here).

About the issue in general: I'm not quite sure if it is really the plugin which is causing the problem. It seems that it is implemented strictly according to the API. I think there is a design flaw in the Trac database API itself because it relies somehow on the garbage collector to close connections. This is a very bad style which is probably causing this issue as a side effect.

Please keep me posted here about your changes you mentioned above. I'm curious if they solve this issue, too.

11/22/12 01:41:02 changed by hasienda

(In [12378]) AccountManagerPlugin: Prevent SSO functionality from stumbling on db issues, refs #9676 and #10625.

I've been exploring seemingly random errors for a couple of months, starting with the introduction of single-sign-on support code in r11127. It turned out to be caused by (SQLite) db access issues concealed behind more than a dozen unique trace-backs, many of them even not yielding a hint on their connection to attempted/prior Trac db access at all.

Sure, the forced environment shutdown was a first good candidate to sort them out, but only turning on Trac environment caching made Trac finally running stable with the new SSO code in place.

As a follow-up on changeset [11131] I've futher reduced the session cookie update frequency after taxing the probability of a session cookie exploit at approximately 1:2*10^38 per forged HTTP request, to reduce db burden by this feature as well.

11/23/12 08:16:26 changed by ms4py

  • cc deleted.

Just did a short test with [12378] and everything seems fine. I'll do a longer test in production and will notify you if anything occurs again.

In this case I think we don't need my patch at all, it was basically just a replacement of db = env.get_db_cnx() by with self.env.db_transaction as db:.

12/01/12 16:55:52 changed by hasienda

  • status changed from new to closed.
  • resolution set to fixed.

(In [12398]) AccountManagerPlugin: Releasing version 0.4, pushing development to acct_mgr-0.5dev.

Availability of that code as stable release closes #874, #3459, #4677, #5295, #5691, #6616, #7577, #8076, #8685, #8770, #8791, #8990, #9052, #9079, #9090, #9139, #9246, #9252, #9547, #9618, #9676, #9843, #9852, #9940, #10023, #10028, #10123, #10142, #10204, #10276, #10397, #10412, #10594, #10625 and #10644.

Some more issues have been worked-on, yet without confirmed resolution, refs #5464 (for JiraToTracIntegration), #8927 and #10134.

And finally there are some issues and enhancement requests showing progress, but known to require more work to resolve them satisfactorily, refs #843, #1600, #5964, #8217, #8933.

Thanks to all contributors and followers, that enabled and encouraged a good portion of this development work.

12/10/12 08:42:47 changed by ms4py

  • status changed from closed to reopened.
  • resolution deleted.

There is another shutdown causing the same issue (in r12378 and in r12398). Not sure why I missed this before. Removing this line should finally fix it:

http://trac-hacks.org/browser/accountmanagerplugin/0.11/acct_mgr/web_ui.py#L614

12/11/12 01:15:13 changed by hasienda

(In [12429]) AccountManagerPlugin: Finally fix db issues for SSO functionality, refs #9676 and #10625.

This is an urgent follow-up to [12378], altering another critical occurrence of cross-environment Trac db access, that I've been overlooking before.

Thanks to ms4py for testing and hinting on this remaining issue.

12/26/12 22:22:18 changed by hasienda

  • status changed from reopened to closed.
  • resolution set to fixed.

(In [12482]) AccountManagerPlugin: Publish maintenance release 0.4.1, closes #5964, #8545, #10134, #10625, #10700 and #10701.

This is an update for current stable acct_mgr-0.4 with a number of fixes for issues resolved within the last weeks, i.e.:

  • a final fix for Single-Sign-On functionality (refs #9676),
  • a long-standing HttpAuth? login issue and
  • one for acct_mgr.LoginModule, that is relevant if used with web-servers, that evaluate the REMOTE_USER environment variable.

Changeset [12468] is included, that may require a Trac db fix-up. Run python ./contrib/fix-session_attribute-failed_logins.py <env> once on any Trac environment, that had account locking enabled with time constraints before.


Add/Change #10625 (AssertionError in trac.db.pool.PooledConnection.__del__)




Change Properties
Action