#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 )
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 12 years ago by
comment:2 Changed 12 years ago by
Cc: | ms4py added; anonymous removed |
---|
comment:3 follow-up: 5 Changed 12 years ago by
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 Changed 12 years ago by
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 follow-up: 7 Changed 12 years ago by
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 12 years ago by
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 Changed 12 years ago by
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 12 years ago by
(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 12 years ago by
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 12 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
(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 12 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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 12 years ago by
(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 12 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
(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.
Well, a cleaner fix would be to upgrade the database access in general.