Modify

Opened 11 years ago

Closed 11 years ago

Last modified 14 months ago

#10625 closed defect (fixed)

AssertionError in trac.db.pool.PooledConnection.__del__

Reported by: anonymous Owned by: Steffen Hoffmann
Priority: normal Component: AccountManagerPlugin
Severity: normal Keywords: database API
Cc: Trac Release: 1.0

Description (last modified by Steffen Hoffmann)

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 (0)

Change History (13)

comment:1 Changed 11 years ago by ms4py

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

comment:2 Changed 11 years ago by ms4py

Cc: ms4py added; anonymous removed

comment:3 Changed 11 years ago 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.

comment:4 in reply to:  description Changed 11 years ago by Steffen Hoffmann

Description: modified (diff)
Keywords: database API added

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.

comment:5 in reply to:  3 ; Changed 11 years ago by Steffen Hoffmann

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.

comment:6 Changed 11 years ago by Steffen Hoffmann

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.

comment:7 in reply to:  5 Changed 11 years ago 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.

comment:8 Changed 11 years ago by Steffen Hoffmann

(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*1038 per forged HTTP request, to reduce db burden by this feature as well.

comment:9 Changed 11 years ago by ms4py

Cc: anonymous added; ms4py removed

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:.

comment:10 Changed 11 years ago by Steffen Hoffmann

Resolution: fixed
Status: newclosed

(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.

comment:11 Changed 11 years ago by ms4py

Resolution: fixed
Status: closedreopened

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

comment:12 Changed 11 years ago by Steffen Hoffmann

(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.

comment:13 Changed 11 years ago by Steffen Hoffmann

Resolution: fixed
Status: reopenedclosed

(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.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Steffen Hoffmann.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.