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.

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

threading.current_thread does not work as expected for plain greenlets #731

Open
melwitt opened this issue Oct 21, 2021 · 8 comments
Open

Comments

@melwitt
Copy link

melwitt commented Oct 21, 2021

I'm running into an issue while using locks from the fasteners library [1][2] in an eventlet monkey patched application.

The fasteners library uses the threading.current_thread method to track which thread is holding a particular lock [3] and it works well for green threads of type GreenThread that are created by calling spawn [4]. However, the application I'm working with creates greenlets by calling spawn_n [5] and in this case threading.current_thread falls back to the standard threading.current_thread method [6] instead of calling eventlet.getcurrent.

Is this expected that the eventlet threading.current_thread method cannot be used with plain greenlets created by spawn_n?

Here is an attempt at sample code demonstrating the above:

import eventlet


eventlet.monkey_patch()


import threading


main = eventlet.getcurrent()


def f():
    print('current_thread = %s' % threading.current_thread())
    print('getcurrent = %s' % eventlet.getcurrent())
    main.switch()


print('\nCalling spawn() ...')
t1 = eventlet.spawn(f)
t1.wait()

print('\nCalling spawn_n() ...')
t2 = eventlet.spawn_n(f)
t2.switch()

print('\nDone')

Output:


Calling spawn() ...
current_thread = <_GreenThread(GreenThread-1, <GreenThread object at 0x7f42b7725880 (otid=0x7f42b788e080) current active started>)>
getcurrent = <GreenThread object at 0x7f42b7725880 (otid=0x7f42b788e080) current active started>

Calling spawn_n() ...
current_thread = <_MainThread(MainThread, started 139924526151488)>
getcurrent = <greenlet.greenlet object at 0x7f42b3c07b40 (otid=0x7f42b788e080) current active started>

Done

Any input would be appreciated.

[1] https://fasteners.readthedocs.io/en/latest
[2] https://pypi.org/project/fasteners
[3] https://github.com/harlowja/fasteners/blob/e1764c7/fasteners/lock.py#L119
[4] https://eventlet.net/doc/modules/greenthread.html#eventlet.greenthread.spawn
[5] https://eventlet.net/doc/modules/greenthread.html#eventlet.greenthread.spawn_n
[6] https://github.com/eventlet/eventlet/blob/8904a33/eventlet/green/threading.py#L125-L128

@temoto
Copy link
Member

temoto commented Oct 28, 2021

Right, spawn_n() looks inconsistent in big picture. I vaguely recall this decision, separation to greenlets and greenthreads was intentional, unsure if the distinction is still useful. spawn_n is intentionally lacking features for speed.

It could be possible to bend current_thread to raw greenlets, but there may be dragons as well. I'd most likely merge any relevant work that doesn't hurt performance; sorry, don't have time to actually code it.

So if spawn_n doesn't fully cover your task, use spawn. Distinction is negligible most of the time. I understand you may not control spawn caller. Your best option is to send them patch. Another option is patch eventlet.spawn_n=spawn, they should be one way compatible.

@melwitt
Copy link
Author

melwitt commented Oct 29, 2021

Thank you very much for the reply, this info will definitely help me find a way forward. Much appreciated!

@melwitt
Copy link
Author

melwitt commented Oct 29, 2021

Apologies that I forgot to mention this earlier ... but the scenario I was most interested in is when threading.current_thread is called from the main greenlet. Is it expected to not be able to call threading.current_thread from code running in the main greenlet of an eventlet monkey patched application?

After thinking about it, I realized that is the problem I'm running into.

@temoto
Copy link
Member

temoto commented Oct 29, 2021

@melwitt why no, current_thread works from anywhere and returns threading.Thread compatible object.

@melwitt
Copy link
Author

melwitt commented Nov 13, 2021

It does ... I'm not sure whether what I'm about to say will make sense, but what I'm referring to is the fact that when called from the main greenlet, current_thread will return a native threading.Thread object and not an object unique to the greenlet. The native object is OK as long as spawn_n is not called from that main greenlet but if it is, the greenlet created by spawn_n will return the same native object as the main greenlet if current_thread is called within it, making the main greenlet and the spawned greenlet appear to be the "same thread" to callers of current_thread. This is the trouble in the scenario I'm looking at currently.

I did some testing and found that the issues do indeed disappear when I replace all spawn_n with spawn. Even though current_thread is still being called in code running in the main greenlet, no other spawned green thread will return the same object as the main greenlet (as there are no other plain greenlets), so there are no problems with two different threads appearing to be the same threading.Thread compatible object when current_thread is used.

Hope that makes sense. Either way, thank you for the helpful guidance.

@temoto
Copy link
Member

temoto commented Nov 13, 2021

@melwitt I understand the inconsistency, it's a shame. I don't have a fast solution though. Sorry.

This would be a very weird workaround crutch:

import eventlet
eventlet.spawn_n = eventlet.spawn
eventlet.convenient.spawn_n = eventlet.spawn
eventlet.monkey_patch()

wouldn't recommend running it in production with random other libraries set, which could depend on spawn_n specifics, unlikely.

@temoto
Copy link
Member

temoto commented Nov 13, 2021

It's somewhat redundant to call current_thread() from main thread, maybe that's why this issue wasn't reported years ago.

@melwitt
Copy link
Author

melwitt commented Nov 16, 2021

Thank you @temoto, that all makes sense.

Meanwhile, I found something else interesting. I tried a local test where I replaced all use of spawn_n() with spawn() in the application I'm working with and found another way spawn_n() can be called indirectly. There is yet a different library being used by the app which creates a thread.Thread to do something and when start() is called on it, eventually start_new_thread() is called and the eventlet version of start_new_thread() calls spawn_n() [1] to create the greenthread. This means if anything running in that thread.Thread calls current_thread(), it will get the same native thread object. Not sure if that is expected also, that eventlet thread.Thread does not work with threading.current_thread().

[1] https://github.com/eventlet/eventlet/blob/v0.32.0/eventlet/green/thread.py#L72

openstack-mirroring pushed a commit to openstack/oslo.concurrency that referenced this issue Sep 6, 2022
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
* 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
* 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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants