ThreadTransactionManager is unsafe in the presence of short-lived threads

Bug #239086 reported by James Henstridge
Affects Status Importance Assigned to Milestone
Launchpad itself
James Henstridge
Won't Fix
Fix Released

Bug Description

The ThreadTransactionManager class from the transaction module manages one transaction per thread. It implements its own thread local storage for the transactions through a dictionary keyed off thread.get_ident().

The thread.get_ident() function returns an identifier for the current thread that is unique during the lifetime of the thread. This works okay for long running threads, but causes problems if transactions are used within short running threads. The value of thread.get_ident() can (and does) get reused by threads that are created one after the other.

On my system (Linux with glibc-2.7), the following test program prints the same thread ID 10 times:

    import thread
    import threading

    def foo():
        print 'Thread ID:', thread.get_ident()

    for i in range(10):
        t = threading.Thread(target=foo)

If these threads had been using the transaction module, they'd have all shared the same transaction. This is causes problems if a previous thread had exited uncleanly and the new thread starts part way through the existing transaction.

Using a threading.local() object to store the per-thread transaction would fix this problem.

Revision history for this message
Jonathan Lange (jml) wrote :

Hey James,

I remember when you found this bug, but I don't remember how it affects launchpad-bazaar. Is this still an issue for us?


Changed in launchpad-bazaar:
assignee: nobody → jamesh
status: New → Incomplete
Revision history for this message
James Henstridge (jamesh) wrote :

The bug shouldn't affect Launchpad in production because it uses a pool of long lived threads. The problems we saw were in tests that were creating/destroying threads.

Revision history for this message
Jonathan Lange (jml) wrote :

Right. The question is, do we need to do anything in Launchpad Bazaar to fix this problem?

AFAICT, we've worked around the issue and now it's up to zodb to correct the underlying bug.

Revision history for this message
James Henstridge (jamesh) wrote :

Yeah, we have a work around to do "transaction.abort" in each of the test's thread pool threads which should avoid the problem.

Revision history for this message
Jonathan Lange (jml) wrote :

AFAICT this isn't a bug anymore.

Changed in launchpad-bazaar:
status: Incomplete → Invalid
Jim Fulton (jim-zope)
Changed in zodb:
importance: Undecided → Medium
status: New → Confirmed
Curtis Hovey (sinzui)
tags: added: tech-debt
Revision history for this message
Jim Fulton (jim-zope) wrote :

This will be released in transaction 1.1. Heck if I know how to get the meta data right.

Changed in zodb:
status: Confirmed → Fix Committed
Changed in zodb:
status: Fix Committed → Fix Released
Tres Seaver (tseaver)
Changed in transaction:
status: New → Fix Released
Changed in zodb:
status: Fix Released → Won't Fix
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers