)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"46b82a17dcb2601ec3027854f8535be2d21e2083","unresolved":true,"context_lines":[{"line_number":16,"context_line":"falls back to the original unpatched current_thread method if it is not"},{"line_number":17,"context_line":"called from a GreenThead [2] and that breaks our tests involving this"},{"line_number":18,"context_line":"fixture. We can work around this by patching threading.current_thread"},{"line_number":19,"context_line":"with eventlet.getcurrent during creation of the lock object."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"[1] https://github.com/harlowja/fasteners/commit/467ed75ee1e9465ebff8b5edf452770befb93913"},{"line_number":22,"context_line":"[2] https://github.com/eventlet/eventlet/blob/v0.32.0/eventlet/green/threading.py#L128"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"f9deac8e_887123b1","line":19,"updated":"2021-10-18 14:57:45.000000000","message":"I need more context to grok this. What exactly breaks in the nova testing if we are not adding this workaround?\n\nI do see oslo_concurrency depends on fasteners directly not nova. Should this WA be applied in oslo_concurrency instead?","commit_id":"244a6ae492abe79266fdf983be5da67cfce681ff"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"83aa0efd104dde9679c4aa16109cd46af9474bc0","unresolved":true,"context_lines":[{"line_number":16,"context_line":"falls back to the original unpatched current_thread method if it is not"},{"line_number":17,"context_line":"called from a GreenThead [2] and that breaks our tests involving this"},{"line_number":18,"context_line":"fixture. We can work around this by patching threading.current_thread"},{"line_number":19,"context_line":"with eventlet.getcurrent during creation of the lock object."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"[1] https://github.com/harlowja/fasteners/commit/467ed75ee1e9465ebff8b5edf452770befb93913"},{"line_number":22,"context_line":"[2] https://github.com/eventlet/eventlet/blob/v0.32.0/eventlet/green/threading.py#L128"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"9d0a5bc0_1b5fed0e","line":19,"in_reply_to":"0ec0076c_3d0b50e3","updated":"2021-10-21 19:20:25.000000000","message":"Thank you gibi for cracking the case yet again. I was way off 😂\n\nI have opened an issue for eventlet in the hope that someone can shed some light on whether this is expected in eventlet:\n\nhttps://github.com/eventlet/eventlet/issues/731\n\nAnd if it is, maybe someone can suggest if there is something different that fasteners could or should be doing.","commit_id":"244a6ae492abe79266fdf983be5da67cfce681ff"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"07634f4b19376c59747e5f9aa2510c00cd66dbd7","unresolved":true,"context_lines":[{"line_number":16,"context_line":"falls back to the original unpatched current_thread method if it is not"},{"line_number":17,"context_line":"called from a GreenThead [2] and that breaks our tests involving this"},{"line_number":18,"context_line":"fixture. We can work around this by patching threading.current_thread"},{"line_number":19,"context_line":"with eventlet.getcurrent during creation of the lock object."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"[1] https://github.com/harlowja/fasteners/commit/467ed75ee1e9465ebff8b5edf452770befb93913"},{"line_number":22,"context_line":"[2] https://github.com/eventlet/eventlet/blob/v0.32.0/eventlet/green/threading.py#L128"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"49a17221_d881bf5b","line":19,"in_reply_to":"294822ee_f944a42e","updated":"2021-10-19 09:31:58.000000000","message":"Thanks. So we have pin for fasteners 0.14.1 that we want to remove. \n\nI\u0027m really afraid of touching the eventlet internals even in our tests as this whole eventlet thing is a complete mess already. I would sleep better if fastener would be patched in our test and not eventlet. \n\nAs of why we have naked greenlets instead of having just GreenThreads I have no idea but I saw that too and it is part of why I think we have a mess.","commit_id":"244a6ae492abe79266fdf983be5da67cfce681ff"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"89186a15d37125d11f903dac625042f711a09242","unresolved":true,"context_lines":[{"line_number":16,"context_line":"falls back to the original unpatched current_thread method if it is not"},{"line_number":17,"context_line":"called from a GreenThead [2] and that breaks our tests involving this"},{"line_number":18,"context_line":"fixture. We can work around this by patching threading.current_thread"},{"line_number":19,"context_line":"with eventlet.getcurrent during creation of the lock object."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"[1] https://github.com/harlowja/fasteners/commit/467ed75ee1e9465ebff8b5edf452770befb93913"},{"line_number":22,"context_line":"[2] https://github.com/eventlet/eventlet/blob/v0.32.0/eventlet/green/threading.py#L128"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"7abffc57_0af86a15","line":19,"in_reply_to":"2d08aa7c_83c46315","updated":"2021-10-21 11:53:35.000000000","message":"I also discovered that there are two python threads exists during our test execution. Both threads has its own main greenlet and its own eventlet.hub greenlet. I\u0027m still trying to find why we need two threads.","commit_id":"244a6ae492abe79266fdf983be5da67cfce681ff"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"361bacaf70e67b5925d0eb02f045d18fad2fa2f9","unresolved":true,"context_lines":[{"line_number":16,"context_line":"falls back to the original unpatched current_thread method if it is not"},{"line_number":17,"context_line":"called from a GreenThead [2] and that breaks our tests involving this"},{"line_number":18,"context_line":"fixture. We can work around this by patching threading.current_thread"},{"line_number":19,"context_line":"with eventlet.getcurrent during creation of the lock object."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"[1] https://github.com/harlowja/fasteners/commit/467ed75ee1e9465ebff8b5edf452770befb93913"},{"line_number":22,"context_line":"[2] https://github.com/eventlet/eventlet/blob/v0.32.0/eventlet/green/threading.py#L128"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"c8081bc0_f764ab29","line":19,"in_reply_to":"49a17221_d881bf5b","updated":"2021-10-20 23:16:03.000000000","message":"\u003e As of why we have naked greenlets instead of having just GreenThreads I have no idea but I saw that too and it is part of why I think we have a mess.\n\nI\u0027ve been reading through eventlet docs and code and I think what we might be seeing is the main greenlet [1], which is the \"root\" or \"parent\" and is not a GreenThread. Weirdly the best doc I found about it is a super old version readme for using it with twisted [2].\n\nAccording to these docs, there should only be one naked greenlet for running main. And it would make sense that is where our tests run from and the only GreenThreads we have are the ones that get spawn()ed from the main greenlet.\n\nSo it seems like eventlet\u0027s threading.current_thread method is not intended to be called by the main greenlet, only from a GreenThread spawned from it. fasteners is using threading.current_thread to track which thread is holding a lock. I am not sure if this would be considered a bug in either library.\n\n[1] https://eventlet.net/doc/hubs.html#how-the-hubs-work\n[2] https://github.com/eventlet/eventlet/blob/v0.15/README.twisted#L149","commit_id":"244a6ae492abe79266fdf983be5da67cfce681ff"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"21d6010f3d77aabf6783c6db244f0d7431a9dd9a","unresolved":true,"context_lines":[{"line_number":16,"context_line":"falls back to the original unpatched current_thread method if it is not"},{"line_number":17,"context_line":"called from a GreenThead [2] and that breaks our tests involving this"},{"line_number":18,"context_line":"fixture. We can work around this by patching threading.current_thread"},{"line_number":19,"context_line":"with eventlet.getcurrent during creation of the lock object."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"[1] https://github.com/harlowja/fasteners/commit/467ed75ee1e9465ebff8b5edf452770befb93913"},{"line_number":22,"context_line":"[2] https://github.com/eventlet/eventlet/blob/v0.32.0/eventlet/green/threading.py#L128"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"0ec0076c_3d0b50e3","line":19,"in_reply_to":"7abffc57_0af86a15","updated":"2021-10-21 12:45:01.000000000","message":"Just a side note on the threads probably unrealted to this patch: \n\nSo the additional python thread is from oslo_messaging[1][2] rpc metrics collections. I think we can ignore that.\n\n[1] https://github.com/openstack/oslo.messaging/blob/feb72de7b81e3919dedc697f9fb5484a92f85ad8/oslo_messaging/rpc/client.py#L185\n[2] https://github.com/openstack/oslo.messaging/blob/feb72de7b81e3919dedc697f9fb5484a92f85ad8/oslo_messaging/_metrics/client.py#L77-L78","commit_id":"244a6ae492abe79266fdf983be5da67cfce681ff"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"e13dc4c11636419398b04d48129c7430dbfb1d42","unresolved":true,"context_lines":[{"line_number":16,"context_line":"falls back to the original unpatched current_thread method if it is not"},{"line_number":17,"context_line":"called from a GreenThead [2] and that breaks our tests involving this"},{"line_number":18,"context_line":"fixture. We can work around this by patching threading.current_thread"},{"line_number":19,"context_line":"with eventlet.getcurrent during creation of the lock object."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"[1] https://github.com/harlowja/fasteners/commit/467ed75ee1e9465ebff8b5edf452770befb93913"},{"line_number":22,"context_line":"[2] https://github.com/eventlet/eventlet/blob/v0.32.0/eventlet/green/threading.py#L128"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"e9b4a179_f3ac8b40","line":19,"in_reply_to":"9d0a5bc0_1b5fed0e","updated":"2021-10-25 12:52:10.000000000","message":"Cool, lets see what the eventlet folks think about this.\n\nI\u0027m also wondering why nova sometimes use spawn, and in other cases use spawn_n. But I did not find any contextual information about this choice from the past.","commit_id":"244a6ae492abe79266fdf983be5da67cfce681ff"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"aedc8417b32191141d04eeb3ff0a6462e97400c7","unresolved":true,"context_lines":[{"line_number":16,"context_line":"falls back to the original unpatched current_thread method if it is not"},{"line_number":17,"context_line":"called from a GreenThead [2] and that breaks our tests involving this"},{"line_number":18,"context_line":"fixture. We can work around this by patching threading.current_thread"},{"line_number":19,"context_line":"with eventlet.getcurrent during creation of the lock object."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"[1] https://github.com/harlowja/fasteners/commit/467ed75ee1e9465ebff8b5edf452770befb93913"},{"line_number":22,"context_line":"[2] https://github.com/eventlet/eventlet/blob/v0.32.0/eventlet/green/threading.py#L128"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"2d08aa7c_83c46315","line":19,"in_reply_to":"c8081bc0_f764ab29","updated":"2021-10-21 09:42:14.000000000","message":"I have a bit different view on naked greenlets. True there is one naked greenlet which is basically the process that started running when we started the interpreter. As it was not spawned by eventlet it cannot be a GreenThread. BUT there are other naked greenlets. \n\nOne is tight to the eventlet.hub (whatever it is) and becomes the parent of each spawned GreenThread (see [1]). The hub.greenlet created here [2] and it is not wrapped in a GreenThread.\n\nAnother source of naked greenlets are the nova use of eventlet.spawn_n() instead of eventlet.spawn(). Based on the code[4] and the doc[3] the spawn_n intentionally does not warp the naked greenlet in a GreenThread for performance reasons (I guess). \n\nTo list running greenlets and GreenThreads I used the trick from the guru mediation code. You can get access to each python object via the GC. e.g:\ngreenlets \u003d [o for o in gc.get_objects() if isinstance(o, greenlet.greenlet)]\ngreenthreads \u003d [o for o in gc.get_objects() if isinstance(o, eventlet.greenthread.GreenThread)]\n\nbottom line: \n\nI think it is not safe to mix spawn_n with ReaderWriterLock as ReaderWriterLock assumes that each entity taking the lock is a GreenThread but in nova we use spawn_n that creates naked greenlets instead of GreenThreads. \n\n[1] https://github.com/eventlet/eventlet/blob/8904a339f2df3bd7958e73142d640cd3d0a9245c/eventlet/greenthread.py#L52\n[2] https://github.com/eventlet/eventlet/blob/8904a339f2df3bd7958e73142d640cd3d0a9245c/eventlet/hubs/hub.py#L130\n[3] https://github.com/eventlet/eventlet/blob/8904a339f2df3bd7958e73142d640cd3d0a9245c/eventlet/greenthread.py#L58-L61\n[4] https://github.com/eventlet/eventlet/blob/8904a339f2df3bd7958e73142d640cd3d0a9245c/eventlet/greenthread.py#L158","commit_id":"244a6ae492abe79266fdf983be5da67cfce681ff"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d7b08e891ce0e41514576012b63eaadb0ed5b893","unresolved":true,"context_lines":[{"line_number":16,"context_line":"falls back to the original unpatched current_thread method if it is not"},{"line_number":17,"context_line":"called from a GreenThead [2] and that breaks our tests involving this"},{"line_number":18,"context_line":"fixture. We can work around this by patching threading.current_thread"},{"line_number":19,"context_line":"with eventlet.getcurrent during creation of the lock object."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"[1] https://github.com/harlowja/fasteners/commit/467ed75ee1e9465ebff8b5edf452770befb93913"},{"line_number":22,"context_line":"[2] https://github.com/eventlet/eventlet/blob/v0.32.0/eventlet/green/threading.py#L128"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"be1a3fc5_e0f7ccfc","line":19,"in_reply_to":"e9b4a179_f3ac8b40","updated":"2021-11-13 02:28:52.000000000","message":"I got a reply from a eventlet maintainer a couple of weeks ago:\n\nhttps://github.com/eventlet/eventlet/issues/731#issuecomment-953761883\n\nand they indicated (IMO) that spawn_n() is not really compatible with eventlet.current_thread() in that all plain greenlets will return the same native Thread object when eventlet.current_thread() is called.\n\nThis leads me to think maybe we should remove spawn_n and use spawn instead so that the environment behaves in a more consistent way and libraries using threading.current_thread work without problems.\n\nThe next PS will be trying out that approach. In my local testing of running the tests, there are no failures with fasteners 0.15 when all of our green thread spawns are done with spawn() instead of spawn_n().","commit_id":"244a6ae492abe79266fdf983be5da67cfce681ff"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"4c5ae865f137229129dcb0bd57e30c966ed1965b","unresolved":true,"context_lines":[{"line_number":16,"context_line":"falls back to the original unpatched current_thread method if it is not"},{"line_number":17,"context_line":"called from a GreenThead [2] and that breaks our tests involving this"},{"line_number":18,"context_line":"fixture. We can work around this by patching threading.current_thread"},{"line_number":19,"context_line":"with eventlet.getcurrent during creation of the lock object."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"[1] https://github.com/harlowja/fasteners/commit/467ed75ee1e9465ebff8b5edf452770befb93913"},{"line_number":22,"context_line":"[2] https://github.com/eventlet/eventlet/blob/v0.32.0/eventlet/green/threading.py#L128"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"294822ee_f944a42e","line":19,"in_reply_to":"f86fe807_e820a223","updated":"2021-10-18 22:11:05.000000000","message":"Note that the only reason we don\u0027t receive the eventlet patched version of current_thread that we need is because our tests are running in greenlets that are not GreenThreads. The eventlet code will only return the patched method if running in a GreenThread and not a plain greenlet. I do not know enough about eventlet to know why they specifically only allow GreenThread for returning the patched version. Could it be a bug in eventlet that it does this? I am not sure.","commit_id":"244a6ae492abe79266fdf983be5da67cfce681ff"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d334d8edf6837f0e895d24b528c963168b36e2db","unresolved":true,"context_lines":[{"line_number":16,"context_line":"falls back to the original unpatched current_thread method if it is not"},{"line_number":17,"context_line":"called from a GreenThead [2] and that breaks our tests involving this"},{"line_number":18,"context_line":"fixture. We can work around this by patching threading.current_thread"},{"line_number":19,"context_line":"with eventlet.getcurrent during creation of the lock object."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"[1] https://github.com/harlowja/fasteners/commit/467ed75ee1e9465ebff8b5edf452770befb93913"},{"line_number":22,"context_line":"[2] https://github.com/eventlet/eventlet/blob/v0.32.0/eventlet/green/threading.py#L128"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"f86fe807_e820a223","line":19,"in_reply_to":"f9deac8e_887123b1","updated":"2021-10-18 22:06:08.000000000","message":"We hit this error [1] (note the date, 2019) if we use fasteners \u003e\u003d 0.15. The reason is explained in more detail in the issue but the tl;dr is that when fasteners needs to grab the current thread, it will get the unpatched version of current_thread instead of the eventlet patched one.\n\nIt\u0027s possible that oslo.concurrency would be a better place for the workaround [2]. I don\u0027t feel sure about it because on one hand it is a generic problem could theoretically occur for any openstack project that is doing eventlet monkey patching ... on the other hand the nova project seems to be the only one facing the problem, for some reason.\n\nAre we doing something wrong/different in nova than other eventlet monkey patched projects?\n\nI\u0027m interested in opinions on what we should do in order to allow openstack/requirements to bump the upper constraint for fasteners.\n\n[1] https://github.com/harlowja/fasteners/issues/36\n[2] https://github.com/openstack/oslo.concurrency/blob/a9ccf0a64deb63e7ea07d232437da797462d8f4b/oslo_concurrency/lockutils.py#L121","commit_id":"244a6ae492abe79266fdf983be5da67cfce681ff"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"11a5c8f4ef7009b6c5ea0644d9e1faac30362136","unresolved":false,"context_lines":[{"line_number":24,"context_line":"functionality. This means with fasteners \u003e\u003d 0.15, any project using,"},{"line_number":25,"context_line":"for example, oslo.concurrency ReaderWriterLock in code that runs in a"},{"line_number":26,"context_line":"plain greenlet created by eventlet.spawn_n() will have locks that do"},{"line_number":27,"context_line":"not work properly."},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"Based on (1) the incompatibility of spawn_n() and current_thread() and"},{"line_number":30,"context_line":"(2) no known reason to use spawn_n() specifically, this replaces all"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"0ac49601_ec4b87b2","line":27,"updated":"2021-11-15 12:18:03.000000000","message":"So if any of our test depends on a ReaderWriterLock directly from the test code then we still can fail as the test code runs in a naked greenlet as it was not spawned with eventlet. The test results shows that we don\u0027t have such test case (or the fault can only happen if two of these naked greenlets are sharing such a lock, but we have only a single naked one). Probably nova in production is safe as both the API requests and the RPC requests are handled in a GreenThread. So only the service startup code has a naked greenlet but those code are not parallel.","commit_id":"58880ff28f375715ae481ed39c1f51db21ef6d0b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"39710c479698743c4a7a04db387de2e3ebe91301","unresolved":true,"context_lines":[{"line_number":24,"context_line":"functionality. This means with fasteners \u003e\u003d 0.15, any project using,"},{"line_number":25,"context_line":"for example, oslo.concurrency ReaderWriterLock in code that runs in a"},{"line_number":26,"context_line":"plain greenlet created by eventlet.spawn_n() will have locks that do"},{"line_number":27,"context_line":"not work properly."},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"Based on (1) the incompatibility of spawn_n() and current_thread() and"},{"line_number":30,"context_line":"(2) no known reason to use spawn_n() specifically, this replaces all"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"52840317_1563c49f","line":27,"in_reply_to":"01c2c2b2_7258fa28","updated":"2021-11-18 13:23:44.000000000","message":"\u003e Apologies for not explaining well that it is not oslo.messaging doing the spawn_n ... it is eventlet. oslo.messaging creates a thread.Thread to do some polling [1] and then calls start() on it [2]. The native Thread.start() method [3] calls start_new_thread() [4][5]. And eventlet has patched start_new_thread() [6] and eventlet calls spawn_n() [7].\n\u003e \n\u003e So this means that any eventlet patched code using thread.Thread will be calling spawn_n() indirectly which will make all those eventlet thread.Thread appear as the same Thread object according to threading.current_thread(). This seems broken to me.\n\u003e \n\u003e There\u0027s nothing we can do in oslo.messaging to avoid the spawn_n() other than not eventlet patching the oslo.messaging library in nova, AFAICT. Maybe that is an option? The thing that bothers me about all of this is that, unless I\u0027m missing something, it shows that there are many ways we could break even when we are doing the right thing and eventlet patching everything, because not all things within the eventlet implementations themselves are compatible together ... \n\u003e \n\nNothing to apologies for. And thanks for explaining it. I don\u0027t think we can simply switch to native threading regarding oslo.messaging that would conflict with other part of the service code that spawning eventlets. I think we have a special handling at such eventlet/native thread boundary in libvirt driver (see nova.virt.libvirt.host.Host._native_thread)","commit_id":"58880ff28f375715ae481ed39c1f51db21ef6d0b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"d83596fa33e667d83e47a21acb85a666bd3069f7","unresolved":true,"context_lines":[{"line_number":24,"context_line":"functionality. This means with fasteners \u003e\u003d 0.15, any project using,"},{"line_number":25,"context_line":"for example, oslo.concurrency ReaderWriterLock in code that runs in a"},{"line_number":26,"context_line":"plain greenlet created by eventlet.spawn_n() will have locks that do"},{"line_number":27,"context_line":"not work properly."},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"Based on (1) the incompatibility of spawn_n() and current_thread() and"},{"line_number":30,"context_line":"(2) no known reason to use spawn_n() specifically, this replaces all"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"cc950549_ef788fea","line":27,"in_reply_to":"0ac49601_ec4b87b2","updated":"2021-11-16 11:26:25.000000000","message":"\u003e Probably nova in production is safe as both the API requests and the RPC requests are handled in a GreenThread. So only the service startup code has a naked greenlet but those code are not parallel.\n\nThe followup showed that oslo.messaging uses thread.Thread that actually calls eventelt.spawn_n() in a monkey patches service. So our RPC handlers might be not safe with a ReaderWriterLock","commit_id":"58880ff28f375715ae481ed39c1f51db21ef6d0b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"98b1a6262e077b4c842892bad75f08a7561ddd6c","unresolved":true,"context_lines":[{"line_number":24,"context_line":"functionality. This means with fasteners \u003e\u003d 0.15, any project using,"},{"line_number":25,"context_line":"for example, oslo.concurrency ReaderWriterLock in code that runs in a"},{"line_number":26,"context_line":"plain greenlet created by eventlet.spawn_n() will have locks that do"},{"line_number":27,"context_line":"not work properly."},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"Based on (1) the incompatibility of spawn_n() and current_thread() and"},{"line_number":30,"context_line":"(2) no known reason to use spawn_n() specifically, this replaces all"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"c10e528d_c89a661b","line":27,"in_reply_to":"a7f099c2_b5b1d2e4","updated":"2021-11-17 13:22:59.000000000","message":"I totally agree with you. I also felt from the github discussion that the eventlet maintainer is not really want to change the behavior of spawn_n or the patched threading.current_thread. So we are stuck. \n\nI\u0027m wondering if we could clearly show the wrong behavior of fastener + eventlet in a small example and post that as an issue to both eventlet and fastener projects. \n\nAlternatively we can propose a patch to oslo.messaging to switch from spawn_n to spawn and see how they react.","commit_id":"58880ff28f375715ae481ed39c1f51db21ef6d0b"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"0f304c9426e0063035af22f06af7309d49ba3918","unresolved":true,"context_lines":[{"line_number":24,"context_line":"functionality. This means with fasteners \u003e\u003d 0.15, any project using,"},{"line_number":25,"context_line":"for example, oslo.concurrency ReaderWriterLock in code that runs in a"},{"line_number":26,"context_line":"plain greenlet created by eventlet.spawn_n() will have locks that do"},{"line_number":27,"context_line":"not work properly."},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"Based on (1) the incompatibility of spawn_n() and current_thread() and"},{"line_number":30,"context_line":"(2) no known reason to use spawn_n() specifically, this replaces all"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"01c2c2b2_7258fa28","line":27,"in_reply_to":"c10e528d_c89a661b","updated":"2021-11-17 17:51:15.000000000","message":"\u003e I totally agree with you. I also felt from the github discussion that the eventlet maintainer is not really want to change the behavior of spawn_n or the patched threading.current_thread. So we are stuck. \n\nI got the same impression from the github discussion that eventlet does not want to change the behavior of threading.current_thread and agree we are stuck.\n\n\u003e I\u0027m wondering if we could clearly show the wrong behavior of fastener + eventlet in a small example and post that as an issue to both eventlet and fastener projects. \n\nI think this is a good idea. I will try to work up a better example (using thread.Thread, see the next paragraphs) and post them to both projects.\n\n\u003e Alternatively we can propose a patch to oslo.messaging to switch from spawn_n to spawn and see how they react.\n\nApologies for not explaining well that it is not oslo.messaging doing the spawn_n ... it is eventlet. oslo.messaging creates a thread.Thread to do some polling [1] and then calls start() on it [2]. The native Thread.start() method [3] calls start_new_thread() [4][5]. And eventlet has patched start_new_thread() [6] and eventlet calls spawn_n() [7].\n\nSo this means that any eventlet patched code using thread.Thread will be calling spawn_n() indirectly which will make all those eventlet thread.Thread appear as the same Thread object according to threading.current_thread(). This seems broken to me.\n\nThere\u0027s nothing we can do in oslo.messaging to avoid the spawn_n() other than not eventlet patching the oslo.messaging library in nova, AFAICT. Maybe that is an option? The thing that bothers me about all of this is that, unless I\u0027m missing something, it shows that there are many ways we could break even when we are doing the right thing and eventlet patching everything, because not all things within the eventlet implementations themselves are compatible together ... \n\n[1] https://github.com/openstack/oslo.messaging/blob/feb72de7b81e3919dedc697f9fb5484a92f85ad8/oslo_messaging/_drivers/base.py#L289\n[2] https://github.com/openstack/oslo.messaging/blob/feb72de7b81e3919dedc697f9fb5484a92f85ad8/oslo_messaging/_drivers/base.py#L296\n[3] https://github.com/python/cpython/blob/9bf2cbc4c498812e14f20d86acb61c53928a5a57/Lib/threading.py#L931\n[4] https://github.com/python/cpython/blob/9bf2cbc4c498812e14f20d86acb61c53928a5a57/Lib/threading.py#L34\n[5] https://github.com/python/cpython/blob/9bf2cbc4c498812e14f20d86acb61c53928a5a57/Lib/threading.py#L950\n[6] https://github.com/eventlet/eventlet/blob/v0.32.0/eventlet/green/thread.py#L47\n[7] https://github.com/eventlet/eventlet/blob/v0.32.0/eventlet/green/thread.py#L72","commit_id":"58880ff28f375715ae481ed39c1f51db21ef6d0b"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"be41daf383f1d7bb68e064f339fa53ae801e3e5b","unresolved":true,"context_lines":[{"line_number":24,"context_line":"functionality. This means with fasteners \u003e\u003d 0.15, any project using,"},{"line_number":25,"context_line":"for example, oslo.concurrency ReaderWriterLock in code that runs in a"},{"line_number":26,"context_line":"plain greenlet created by eventlet.spawn_n() will have locks that do"},{"line_number":27,"context_line":"not work properly."},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"Based on (1) the incompatibility of spawn_n() and current_thread() and"},{"line_number":30,"context_line":"(2) no known reason to use spawn_n() specifically, this replaces all"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"a7f099c2_b5b1d2e4","line":27,"in_reply_to":"cc950549_ef788fea","updated":"2021-11-16 20:44:38.000000000","message":"Yeah ... the troubling part about it is that the spawn_n() is being done by eventlet in patched thread.Thread (oslo.messaging is not intentionally doing it). I\u0027m not sure I understand why they would want an eventlet thread to not be able to be uniquely identified by eventlet threading.current_thread(). As I mentioned on the github issue, that means that all eventlet patched thread.Thread that are created will appear to be the same thread according to eventlet\u0027s threading.current_thread() return value.\n\nI am not sure what to do at this point other than to stop using eventlet altogether. I don\u0027t feel good about the fact that any library using threading.current_thread() can have undesired behavior with eventlet patched code, where multiple different green threads will appear as one thread object to current_thread().\n\nOn the github issue the maintainer showed a hacky way of patching spawn_n() \u003d\u003e spawn() globally before calling eventlet.monkey_patch() [1] but he said don\u0027t do that in production code. So that does not seem to be an option either.\n\n[1] https://github.com/eventlet/eventlet/issues/731#issuecomment-968135262","commit_id":"58880ff28f375715ae481ed39c1f51db21ef6d0b"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"46b82a17dcb2601ec3027854f8535be2d21e2083","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"0ef5cdb9_b256b3a1","updated":"2021-10-18 14:57:45.000000000","message":"I have questions","commit_id":"244a6ae492abe79266fdf983be5da67cfce681ff"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d8beaba5cf28843c307b42a2d486b19b64c7745a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"dc47bf1b_54df37ab","updated":"2021-12-09 22:54:19.000000000","message":"Need to open issues for eventlet and fasteners","commit_id":"58880ff28f375715ae481ed39c1f51db21ef6d0b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"11a5c8f4ef7009b6c5ea0644d9e1faac30362136","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"dd914943_d461e36b","updated":"2021-11-15 12:18:03.000000000","message":"Overall looks good to me. Thank you Melanie for solving this! The poisoning of GreenPool.spawn_n in test could be a followup.","commit_id":"58880ff28f375715ae481ed39c1f51db21ef6d0b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"d83596fa33e667d83e47a21acb85a666bd3069f7","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"63aef681_774282bc","updated":"2021-11-16 11:26:25.000000000","message":"dropping my +2 as oslo.messaging could break us still","commit_id":"58880ff28f375715ae481ed39c1f51db21ef6d0b"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"c9c302a727f389cc9217110ebf63152c85fee153","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"dcfa7b3b_0dd9de31","updated":"2022-01-10 12:48:10.000000000","message":"Holding off on +W to ensure melwitt sees gibi\u0027s comments","commit_id":"50bf252250a35f7d9d09c75243f80a4ec0a0fc39"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"5f712e4a6612ef6ec12de335033daa4b0e85dad4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"1f75b78d_9cce01e4","updated":"2022-01-10 12:23:33.000000000","message":"While I don\u0027t like this we run out of other options. My comment about the wrapper and hacking rule can be solved in a followup commit.\n\nCheers,\ngibi","commit_id":"50bf252250a35f7d9d09c75243f80a4ec0a0fc39"}],"nova/hacking/checks.py":[{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"11a5c8f4ef7009b6c5ea0644d9e1faac30362136","unresolved":true,"context_lines":[{"line_number":89,"context_line":"http_not_implemented_re \u003d re.compile(r\"raise .*HTTPNotImplemented\\(\")"},{"line_number":90,"context_line":"spawn_re \u003d re.compile("},{"line_number":91,"context_line":"    r\".*(eventlet|greenthread)\\.(?P\u003cspawn_part\u003espawn(_n)?)\\(.*\\)\")"},{"line_number":92,"context_line":"spawn_n_re \u003d re.compile(r\".*(eventlet|greenthread)\\.spawn_n\\(.*\\)\")"},{"line_number":93,"context_line":"contextlib_nested \u003d re.compile(r\"^with (contextlib\\.)?nested\\(\")"},{"line_number":94,"context_line":"doubled_words_re \u003d re.compile("},{"line_number":95,"context_line":"    r\"\\b(then?|[iao]n|i[fst]|but|f?or|at|and|[dt]o)\\s+\\1\\b\")"}],"source_content_type":"text/x-python","patch_set":3,"id":"da8c1d2f_981c1bad","line":92,"updated":"2021-11-15 12:18:03.000000000","message":"cool this catches most of the future offenders. However it does not catch GreenPool().spawn_n(). I\u0027m wondering if we need to poison it during our tests to catch if that is used","commit_id":"58880ff28f375715ae481ed39c1f51db21ef6d0b"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"0123fb065518e8712b25845ef9aca7512226a045","unresolved":true,"context_lines":[{"line_number":89,"context_line":"http_not_implemented_re \u003d re.compile(r\"raise .*HTTPNotImplemented\\(\")"},{"line_number":90,"context_line":"spawn_re \u003d re.compile("},{"line_number":91,"context_line":"    r\".*(eventlet|greenthread)\\.(?P\u003cspawn_part\u003espawn(_n)?)\\(.*\\)\")"},{"line_number":92,"context_line":"spawn_n_re \u003d re.compile(r\".*(eventlet|greenthread)\\.spawn_n\\(.*\\)\")"},{"line_number":93,"context_line":"contextlib_nested \u003d re.compile(r\"^with (contextlib\\.)?nested\\(\")"},{"line_number":94,"context_line":"doubled_words_re \u003d re.compile("},{"line_number":95,"context_line":"    r\"\\b(then?|[iao]n|i[fst]|but|f?or|at|and|[dt]o)\\s+\\1\\b\")"}],"source_content_type":"text/x-python","patch_set":3,"id":"272a8d59_3eb66535","line":92,"in_reply_to":"da8c1d2f_981c1bad","updated":"2021-11-16 03:54:57.000000000","message":"Good spot, thanks! I have added a followup change on top of this one to try poisoning all spawn_n() in tests.","commit_id":"58880ff28f375715ae481ed39c1f51db21ef6d0b"}],"nova/tests/fixtures/nova.py":[{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"361bacaf70e67b5925d0eb02f045d18fad2fa2f9","unresolved":true,"context_lines":[{"line_number":403,"context_line":"        with (contextlib.ExitStack() if not eventlet_patched else"},{"line_number":404,"context_line":"              fixtures.MonkeyPatch(\u0027threading.current_thread\u0027,"},{"line_number":405,"context_line":"                                   eventlet.getcurrent)):"},{"line_number":406,"context_line":"            self._cell_lock \u003d lockutils.ReaderWriterLock()"},{"line_number":407,"context_line":""},{"line_number":408,"context_line":"    def _cache_schema(self, connection_str):"},{"line_number":409,"context_line":"        # NOTE(melwitt): See the regular Database fixture for why"}],"source_content_type":"text/x-python","patch_set":1,"id":"3a40d7f3_42da4430","line":406,"updated":"2021-10-20 23:16:03.000000000","message":"What I am doing here is temporarily swapping threading.current_thread with evenlet.getcurrent (if thread-related modules have been eventlet patched) while creating the lock object only. In this regard, I considered this to be patching only fasteners, but I could also do it another way like:\n\n self._cell_lock \u003d lockutils.ReaderWriterLock()\n eventlet_patched \u003d eventlet.patcher.is_monkey_patched(\u0027thread\u0027)\n if eventlet_patched:\n     self._cell_lock._current_thread \u003d eventlet.getcurrent\n\nif you prefer.\n\nAnd if there is yet another way that I have missed that you would prefer, please let me know.","commit_id":"244a6ae492abe79266fdf983be5da67cfce681ff"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"11a5c8f4ef7009b6c5ea0644d9e1faac30362136","unresolved":false,"context_lines":[{"line_number":1110,"context_line":"    def setUp(self):"},{"line_number":1111,"context_line":"        super(SpawnIsSynchronousFixture, self).setUp()"},{"line_number":1112,"context_line":"        self.useFixture(fixtures.MonkeyPatch("},{"line_number":1113,"context_line":"            \u0027nova.utils.spawn_n\u0027, _FakeGreenThread))"},{"line_number":1114,"context_line":"        self.useFixture(fixtures.MonkeyPatch("},{"line_number":1115,"context_line":"            \u0027nova.utils.spawn\u0027, _FakeGreenThread))"},{"line_number":1116,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"f465ebfc_8c391502","side":"PARENT","line":1113,"updated":"2021-11-15 12:18:03.000000000","message":"hehe, this was a lie anyhow.","commit_id":"e28afc564700a1a35e3bf0269687d5734251b88a"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"5f712e4a6612ef6ec12de335033daa4b0e85dad4","unresolved":true,"context_lines":[{"line_number":423,"context_line":"        eventlet_patched \u003d eventlet.patcher.is_monkey_patched(\u0027thread\u0027)"},{"line_number":424,"context_line":"        with (contextlib.ExitStack() if not eventlet_patched else"},{"line_number":425,"context_line":"              fixtures.MonkeyPatch(\u0027threading.current_thread\u0027,"},{"line_number":426,"context_line":"                                   eventlet.getcurrent)):"},{"line_number":427,"context_line":"            self._cell_lock \u003d lockutils.ReaderWriterLock()"},{"line_number":428,"context_line":""},{"line_number":429,"context_line":"    def _cache_schema(self, connection_str):"}],"source_content_type":"text/x-python","patch_set":4,"id":"5d6d9370_05da42c3","line":426,"updated":"2022-01-10 12:23:33.000000000","message":"So this restores the workaround that is removed from fasteners \u003e\u003d 0.15 to avoid breaking the lock. Melanie tried to convince both fasteners and eventlet maintainers to fix the issue without any luck. It seems that if we want to keep using the RW lock in an eventlet patched environment like nova then the we have to manually fix eventlet (or fasteners) by ourselves. This is sub-optimal I don\u0027t see any viable options that is in our circle of influence. So I\u0027m OK to merge this.\n\n@Melanie: could you add a wrapper for ReaderWriterLock that automatically applies this workaround when the lock is created and add a hacking rule that detects if a patch tries to create a new RW lock with lockutils.ReaderWriterLock() directly instead of using the wrapper? This would prevent future fault when a new RW lock instance is being added to the nova code.","commit_id":"50bf252250a35f7d9d09c75243f80a4ec0a0fc39"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"0eef88604df517785f7a6657b01247c5e36e0b98","unresolved":true,"context_lines":[{"line_number":423,"context_line":"        eventlet_patched \u003d eventlet.patcher.is_monkey_patched(\u0027thread\u0027)"},{"line_number":424,"context_line":"        with (contextlib.ExitStack() if not eventlet_patched else"},{"line_number":425,"context_line":"              fixtures.MonkeyPatch(\u0027threading.current_thread\u0027,"},{"line_number":426,"context_line":"                                   eventlet.getcurrent)):"},{"line_number":427,"context_line":"            self._cell_lock \u003d lockutils.ReaderWriterLock()"},{"line_number":428,"context_line":""},{"line_number":429,"context_line":"    def _cache_schema(self, connection_str):"}],"source_content_type":"text/x-python","patch_set":4,"id":"2de2f5b5_56e70784","line":426,"in_reply_to":"2a580e90_673f5993","updated":"2022-01-12 04:18:06.000000000","message":"Followup change has been proposed here:\n\nhttps://review.opendev.org/c/openstack/nova/+/824280","commit_id":"50bf252250a35f7d9d09c75243f80a4ec0a0fc39"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"065bd1c6352ffc61e1b4d24d3767ce07dc2e921f","unresolved":true,"context_lines":[{"line_number":423,"context_line":"        eventlet_patched \u003d eventlet.patcher.is_monkey_patched(\u0027thread\u0027)"},{"line_number":424,"context_line":"        with (contextlib.ExitStack() if not eventlet_patched else"},{"line_number":425,"context_line":"              fixtures.MonkeyPatch(\u0027threading.current_thread\u0027,"},{"line_number":426,"context_line":"                                   eventlet.getcurrent)):"},{"line_number":427,"context_line":"            self._cell_lock \u003d lockutils.ReaderWriterLock()"},{"line_number":428,"context_line":""},{"line_number":429,"context_line":"    def _cache_schema(self, connection_str):"}],"source_content_type":"text/x-python","patch_set":4,"id":"677becf4_42a3a5ae","line":426,"in_reply_to":"5d6d9370_05da42c3","updated":"2022-01-10 21:00:03.000000000","message":"\u003e @Melanie: could you add a wrapper for ReaderWriterLock that automatically applies this workaround when the lock is created and add a hacking rule that detects if a patch tries to create a new RW lock with lockutils.ReaderWriterLock() directly instead of using the wrapper? This would prevent future fault when a new RW lock instance is being added to the nova code.\n\nYes, this is a good idea and I will add it as a followup commit per your summary comment.","commit_id":"50bf252250a35f7d9d09c75243f80a4ec0a0fc39"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"7b3ea79f0e8f91ea2f246f9f8f8bb6e82129ef15","unresolved":true,"context_lines":[{"line_number":423,"context_line":"        eventlet_patched \u003d eventlet.patcher.is_monkey_patched(\u0027thread\u0027)"},{"line_number":424,"context_line":"        with (contextlib.ExitStack() if not eventlet_patched else"},{"line_number":425,"context_line":"              fixtures.MonkeyPatch(\u0027threading.current_thread\u0027,"},{"line_number":426,"context_line":"                                   eventlet.getcurrent)):"},{"line_number":427,"context_line":"            self._cell_lock \u003d lockutils.ReaderWriterLock()"},{"line_number":428,"context_line":""},{"line_number":429,"context_line":"    def _cache_schema(self, connection_str):"}],"source_content_type":"text/x-python","patch_set":4,"id":"2a580e90_673f5993","line":426,"in_reply_to":"677becf4_42a3a5ae","updated":"2022-01-11 15:16:03.000000000","message":"thanks!","commit_id":"50bf252250a35f7d9d09c75243f80a4ec0a0fc39"}]}