Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fastners ReaderWriterLock is unsafe with eventlets #96

Closed
SeanMooney opened this issue Sep 5, 2022 · 1 comment
Closed

fastners ReaderWriterLock is unsafe with eventlets #96

SeanMooney opened this issue Sep 5, 2022 · 1 comment

Comments

@SeanMooney
Copy link

SeanMooney commented Sep 5, 2022 •
edited

The ReaderWriterLock currently makes heavy use of
threading.current_thread to provide rentrancy.

the pattern in use either check if the pending writer is me or if the current acquire of the lock is the writer

however when used with eventlet and spawn_n that might return the python thread instead of the
eventlet greenthread id and allow different greenthread on the same OS thread to enter the critical section.

we have reproduced this in nova and now oslo.cuncrraency
https://review.opendev.org/c/openstack/oslo.concurrency/+/855713

it is likely because this workaround
467ed75
was removed in 0.15

when the issue was fixed in eventlet it did not cover all edge cases.
eventlet/eventlet#731

melanie noted that using spwan_n threading.current_thread() will return the native thread object. eventlet/eventlet#731 (comment)

so as a result the ReaderWriterLock in all versions of fasteners after 0.15 is unsafe to use with eventlet if the calling application uses spawn_n. This usage may be indirect or explicit in the client application so they may not be aware of this.
@SeanMooney SeanMooney changed the title fastners reader writer locks are unsafe with eventlets fastners ReaderWriterLock is unsafe with eventlets Sep 5, 2022
@SeanMooney SeanMooney mentioned this issue Sep 5, 2022
Closed
openstack-mirroring pushed a commit to openstack/oslo.concurrency that referenced this issue Sep 6, 2022
@gibizer
Prove that spawn_n with fair lock is broken
796203c 
The fasteners lib in version 0.15.0 removed the
threading.current_thread workaround for eventlet[1] because eventlet
seemed to fixed the current_thread issues tracked in [2]. However the
fix for [2] only fixed half of the problem. The threading.current_thread
call works if it is called from thread created by eventlet.spawn.
However if the thread is created with eventlet.spawn_n then
threading.current_thread is still broken and returns the ID of the
python native thread.

The fasteners' ReaderWriterLock depends heavily on
threading.current_thread to decide which thread holds a lock and to
allow re-entry of that thread. This leads to the situation that
multiple threads created from spawn_n could take the same
ReaderWriterLock at the same time.

The fair internal lock in oslo.concurrency uses ReaderWriterLock and
as a result such lock is broken for threads created with spawn_n.

Note that this issue was raised with eventlet in [3] when the nova team
detected it via a direct usage of ReaderWriterLock in the nova test
code. As [3] did not lead to a solution in eventlet nova implemented a
nova local fix for the test code in [4].

However now we detected that oslo.concurrency is affected by this issue
as well.

This patch adds tests to show the scope of the problem.

Note that the coverage tox target is changed to explicitly enable native
threading otherwise it runs eventlet specific tests in a native
environment.

Also note that [5] was opened to reintroduce the workaround[1] in fasteners.

[1] harlowja/fasteners@467ed75
[2] eventlet/eventlet#172
[3] eventlet/eventlet#731
[4] https://review.opendev.org/c/openstack/nova/+/813114
[5] harlowja/fasteners#96

Related-Bug: #1988311
Change-Id: Ibc193c855b49b95b46ebd2aac82ea89e33f885f0
openstack-mirroring pushed a commit to openstack/openstack that referenced this issue Sep 6, 2022
@gibizer @openstack-gerrit
Update git submodules
5b4894e 
* Update oslo.concurrency from branch 'master'
  to 796203c94846d44237d869e3b20a6cd8a3602e36
  - Prove that spawn_n with fair lock is broken
    
    The fasteners lib in version 0.15.0 removed the
    threading.current_thread workaround for eventlet[1] because eventlet
    seemed to fixed the current_thread issues tracked in [2]. However the
    fix for [2] only fixed half of the problem. The threading.current_thread
    call works if it is called from thread created by eventlet.spawn.
    However if the thread is created with eventlet.spawn_n then
    threading.current_thread is still broken and returns the ID of the
    python native thread.
    
    The fasteners' ReaderWriterLock depends heavily on
    threading.current_thread to decide which thread holds a lock and to
    allow re-entry of that thread. This leads to the situation that
    multiple threads created from spawn_n could take the same
    ReaderWriterLock at the same time.
    
    The fair internal lock in oslo.concurrency uses ReaderWriterLock and
    as a result such lock is broken for threads created with spawn_n.
    
    Note that this issue was raised with eventlet in [3] when the nova team
    detected it via a direct usage of ReaderWriterLock in the nova test
    code. As [3] did not lead to a solution in eventlet nova implemented a
    nova local fix for the test code in [4].
    
    However now we detected that oslo.concurrency is affected by this issue
    as well.
    
    This patch adds tests to show the scope of the problem.
    
    Note that the coverage tox target is changed to explicitly enable native
    threading otherwise it runs eventlet specific tests in a native
    environment.
    
    Also note that [5] was opened to reintroduce the workaround[1] in fasteners.
    
    [1] harlowja/fasteners@467ed75
    [2] eventlet/eventlet#172
    [3] eventlet/eventlet#731
    [4] https://review.opendev.org/c/openstack/nova/+/813114
    [5] harlowja/fasteners#96
    
    Related-Bug: #1988311
    Change-Id: Ibc193c855b49b95b46ebd2aac82ea89e33f885f0
openstack-mirroring pushed a commit to openstack/openstack that referenced this issue Sep 6, 2022
@gibizer @openstack-gerrit
Update git submodules
33fe743 
* Update oslo.concurrency from branch 'master'
  to ee3f73a13379a79282325906787aae7da0f6ac27
  - Fix fair internal lock used from eventlet.spawn_n
    
    The fasteners lib in version 0.15.0 removed the
    threading.current_thread workaround for eventlet[1] because eventlet
    seemed to fixed the current_thread issues tracked in [2]. However the
    fix for [2] only fixed half of the problem. The threading.current_thread
    call works if it is called from thread created by eventlet.spawn.
    However if the thread is created with eventlet.spawn_n then
    threading.current_thread is still broken and returns the ID of the
    python native thread.
    
    The fasteners' ReaderWriterLock depends heavily on
    threading.current_thread to decide which thread holds a lock and to
    allow re-entry of that thread. This leads to the situation that
    multiple threads created from spawn_n could take the same
    ReaderWriterLock at the same time.
    
    The fair internal lock in oslo.concurrency uses ReaderWriterLock and
    as a result such lock is broken for threads created with spawn_n.
    
    Note that this issue was raised with eventlet in [3] when the nova team
    detected it via a direct usage of ReaderWriterLock in the nova test
    code. As [3] did not lead to a solution in eventlet nova implemented a
    nova local fix for the test code in [4].
    
    However now we detected that oslo.concurrency is affected by this issue
    as well.
    
    This patch restores the workaround that was removed by [1].
    
    Note that a fasteners issue [5] also opened to restore the
    workaround[1].
    
    [1] harlowja/fasteners@467ed75
    [2] eventlet/eventlet#172
    [3] eventlet/eventlet#731
    [4] https://review.opendev.org/c/openstack/nova/+/813114
    [5] harlowja/fasteners#96
    
    Closes-Bug: #1988311
    Change-Id: Ia873bcc6b07121c9bd0b94c593567d537b4c1112
openstack-mirroring pushed a commit to openstack/oslo.concurrency that referenced this issue Sep 6, 2022
@gibizer
Fix fair internal lock used from eventlet.spawn_n
ee3f73a 
The fasteners lib in version 0.15.0 removed the
threading.current_thread workaround for eventlet[1] because eventlet
seemed to fixed the current_thread issues tracked in [2]. However the
fix for [2] only fixed half of the problem. The threading.current_thread
call works if it is called from thread created by eventlet.spawn.
However if the thread is created with eventlet.spawn_n then
threading.current_thread is still broken and returns the ID of the
python native thread.

The fasteners' ReaderWriterLock depends heavily on
threading.current_thread to decide which thread holds a lock and to
allow re-entry of that thread. This leads to the situation that
multiple threads created from spawn_n could take the same
ReaderWriterLock at the same time.

The fair internal lock in oslo.concurrency uses ReaderWriterLock and
as a result such lock is broken for threads created with spawn_n.

Note that this issue was raised with eventlet in [3] when the nova team
detected it via a direct usage of ReaderWriterLock in the nova test
code. As [3] did not lead to a solution in eventlet nova implemented a
nova local fix for the test code in [4].

However now we detected that oslo.concurrency is affected by this issue
as well.

This patch restores the workaround that was removed by [1].

Note that a fasteners issue [5] also opened to restore the
workaround[1].

[1] harlowja/fasteners@467ed75
[2] eventlet/eventlet#172
[3] eventlet/eventlet#731
[4] https://review.opendev.org/c/openstack/nova/+/813114
[5] harlowja/fasteners#96

Closes-Bug: #1988311
Change-Id: Ia873bcc6b07121c9bd0b94c593567d537b4c1112
SeanMooney added a commit to SeanMooney/fasteners that referenced this issue Sep 6, 2022
@SeanMooney
reintoduce current_thread_functor
80f7aa1 
This change reintoruced the current_thread_functor to the
ReaderWriterLock __init__. This parmater now default to None
and if not passed will result in threading.current_thread being
used to detect if its safe for the current execution context to
renter a lock. If you are using fasterns ReaderWriterLocks in a
context where eventlet or other user land threads are in use, a
alternitive functor should be provided.

Closes: harlowja#96
SeanMooney added a commit to SeanMooney/fasteners that referenced this issue Sep 6, 2022
@SeanMooney
reintoduce current_thread_functor
a2aef02 
This change reintoruced the current_thread_functor to the
ReaderWriterLock __init__. This parmater now default to None
and if not passed will result in threading.current_thread being
used to detect if its safe for the current execution context to
renter a lock. If you are using fasterns ReaderWriterLocks in a
context where eventlet or other user land threads are in use, a
alternitive functor should be provided.

Closes: harlowja#96
@psarka psarka mentioned this issue Sep 13, 2022
Merged
@psarka
Copy link
Collaborator

psarka commented Sep 14, 2022

Should be resolved in version 0.18 🎉

@psarka psarka closed this as completed Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

support eventlet spawn_n SeanMooney/fasteners
2 participants
@psarka @SeanMooney