Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #1419 (closed defect: fixed)

Opened 12 years ago

Last modified 11 years ago

[PATCH] session.clear needed before each request

Reported by: paj Owned by: paj
Priority: high Milestone: 1.0.4
Component: SQLAlchemy Version: 1.0.2
Severity: normal Keywords:
Cc:

Description

I have noticed a bug with a TurboGears/SA app, it seemed to be picking up old data values on occasions. I believe this is due to the SA identity map, and in short I think the answer it to call session.clear() before each request.

I'm attaching a test program that demonstrates the problem in a simple, threaded SA app. With the session.clear commented out, thread2 outputs "hello". With session.clear included, it correctly outputs "hello!".

I'm also attaching my proposed fix, which is actually quite simple. I haven't noticed the problem occuring in my app now, although it's hard to know for sure if it's fixed.

Attachments

idmap.patch Download (485 bytes) - added by paj 12 years ago.
Proposed fix
idmap-test.py Download (879 bytes) - added by paj 12 years ago.
Test case (SA only - not TG)
idmap2.patch Download (449 bytes) - added by paj 12 years ago.
idmap3.patch Download (683 bytes) - added by chrisz 11 years ago.
Alternative patch

Change History

Changed 12 years ago by paj

Proposed fix

Changed 12 years ago by paj

Test case (SA only - not TG)

comment:1 Changed 12 years ago by faide

Anyone has ideas about this ? Seen from where I stand it seems sane but I'm not too sure of the potential impacts...

comment:2 Changed 12 years ago by faide

  • Summary changed from [patch] session.clear needed before each request to [PATCH] session.clear needed before each request

comment:3 Changed 12 years ago by paj

This fix is working great for an app I have live. In fact, I think it's really important to apply a fix - the data freshness problems had become quite an issue.

Mike Orr/Bayer? have mentioned that deleting the session is preferable to clearing it. I believe we would use "del session.context.current". I will have a go at that tomorrow and see how it fares.

Changed 12 years ago by paj

comment:4 Changed 12 years ago by paj

  • Owner changed from anonymous to paj

Yep, the approach of deleting the session works too, and it's the one recommended by the SA author. Patch attached as idmap2.patch.

comment:5 Changed 12 years ago by paj

  • Status changed from new to closed
  • Resolution set to fixed

comment:6 Changed 12 years ago by paj

Fixed in [3290]

comment:7 Changed 12 years ago by chrisz

  • Status changed from closed to reopened
  • Resolution fixed deleted

I think this needs to be reconsidered. See ticket #1452.

comment:8 Changed 12 years ago by Chris Arndt

  • Milestone changed from 1.0.3 to 1.0.4

Batch promoting 1.0.3 tickets to Milestone 1.0.4

comment:9 Changed 12 years ago by chrisz

Can you provide a test for reproducing this problem in TG? Your test only demonstrates a peculiarity of SA but does not help to decide whether a fix solves the issue in TG or not. I suggest to always accompany patches with respective regression tests. See #1452 for an alternative patch, but I cannot check whether this solves your issue here.

Changed 11 years ago by chrisz

Alternative patch

comment:10 Changed 11 years ago by paj

  • Status changed from reopened to closed
  • Resolution set to fixed

Sorry for the delay in looking at this. I have just added a test in changeset [3403]. This was a pretty tough one to write! I'm afraid the proposed idmap3.patch does not fix this issue. I'm going to close this ticket now, but keep #1452 open.

comment:11 Changed 11 years ago by paj

I've made a right hash of this one. TG already had this included, in EndTransactionFilter?. My app had inadvertently removed that filter. The unit test passes even without my original change. In changeset [3405] I have reverted the change and that will fix #1452.

Note: See TracTickets for help on using tickets.