)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"32b7c4ee4042bb17f943a5bd129df8007f81d196","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"68e84b78_73e9d4ed","updated":"2022-03-23 19:04:38.000000000","message":"This addresses the note in there, but I think we might want to be a little more careful about how hard we\u0027re retrying.","commit_id":"6ad153401ec18688cf9732adb1fffa6a3397b43e"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6bd30033b40018f08c8935018dc4225d490e80e9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"65e8cd2e_567d9928","updated":"2022-03-24 19:06:17.000000000","message":"es bueno","commit_id":"1c8122a25f50b40934af127d7717b55794ff38b5"}],"nova/tests/fixtures/nova.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"32b7c4ee4042bb17f943a5bd129df8007f81d196","unresolved":true,"context_lines":[{"line_number":447,"context_line":"        #     to make that change, which assumes that nothing else is looking"},{"line_number":448,"context_line":"        #     at a cell right now. We do only non-schedulable things while"},{"line_number":449,"context_line":"        #     holding that lock to avoid the deadlock mentioned above."},{"line_number":450,"context_line":"        #  3. We then re-lock with a reader lock just as step #1 above and"},{"line_number":451,"context_line":"        #     yield to do the actual work. We can do schedulable things"},{"line_number":452,"context_line":"        #     here and not exclude other threads from making progress."},{"line_number":453,"context_line":"        #     If an exception is raised, we capture that and save it."}],"source_content_type":"text/x-python","patch_set":1,"id":"39633986_1cb18f17","line":450,"updated":"2022-03-23 19:04:38.000000000","message":"Probably worth adding a couple words here to indicate the potential for a retry, especially if you cap it. Maybe the potential failure should have been noted here too, but...","commit_id":"6ad153401ec18688cf9732adb1fffa6a3397b43e"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"020e2b6f1a5b9d342f55fd5d5e92aa90dad775f0","unresolved":true,"context_lines":[{"line_number":447,"context_line":"        #     to make that change, which assumes that nothing else is looking"},{"line_number":448,"context_line":"        #     at a cell right now. We do only non-schedulable things while"},{"line_number":449,"context_line":"        #     holding that lock to avoid the deadlock mentioned above."},{"line_number":450,"context_line":"        #  3. We then re-lock with a reader lock just as step #1 above and"},{"line_number":451,"context_line":"        #     yield to do the actual work. We can do schedulable things"},{"line_number":452,"context_line":"        #     here and not exclude other threads from making progress."},{"line_number":453,"context_line":"        #     If an exception is raised, we capture that and save it."}],"source_content_type":"text/x-python","patch_set":1,"id":"b6c3327a_e7462af1","line":450,"in_reply_to":"1288c665_95be7fa6","updated":"2022-03-23 19:23:55.000000000","message":"Yeah, since whoever wrote this code left it out in the first place :P","commit_id":"6ad153401ec18688cf9732adb1fffa6a3397b43e"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"939018e3250fca024d324e8c7c256722624686d4","unresolved":true,"context_lines":[{"line_number":447,"context_line":"        #     to make that change, which assumes that nothing else is looking"},{"line_number":448,"context_line":"        #     at a cell right now. We do only non-schedulable things while"},{"line_number":449,"context_line":"        #     holding that lock to avoid the deadlock mentioned above."},{"line_number":450,"context_line":"        #  3. We then re-lock with a reader lock just as step #1 above and"},{"line_number":451,"context_line":"        #     yield to do the actual work. We can do schedulable things"},{"line_number":452,"context_line":"        #     here and not exclude other threads from making progress."},{"line_number":453,"context_line":"        #     If an exception is raised, we capture that and save it."}],"source_content_type":"text/x-python","patch_set":1,"id":"1288c665_95be7fa6","line":450,"in_reply_to":"39633986_1cb18f17","updated":"2022-03-23 19:20:35.000000000","message":"Sounds like a good idea, will add mention of the retry and failure potential.","commit_id":"6ad153401ec18688cf9732adb1fffa6a3397b43e"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d8344b76c7c60783fbeaa19eb1828f89cbd0c61e","unresolved":false,"context_lines":[{"line_number":447,"context_line":"        #     to make that change, which assumes that nothing else is looking"},{"line_number":448,"context_line":"        #     at a cell right now. We do only non-schedulable things while"},{"line_number":449,"context_line":"        #     holding that lock to avoid the deadlock mentioned above."},{"line_number":450,"context_line":"        #  3. We then re-lock with a reader lock just as step #1 above and"},{"line_number":451,"context_line":"        #     yield to do the actual work. We can do schedulable things"},{"line_number":452,"context_line":"        #     here and not exclude other threads from making progress."},{"line_number":453,"context_line":"        #     If an exception is raised, we capture that and save it."}],"source_content_type":"text/x-python","patch_set":1,"id":"0d80ef6d_823ccbf1","line":450,"in_reply_to":"b6c3327a_e7462af1","updated":"2022-03-24 18:47:09.000000000","message":"Done","commit_id":"6ad153401ec18688cf9732adb1fffa6a3397b43e"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"32b7c4ee4042bb17f943a5bd129df8007f81d196","unresolved":true,"context_lines":[{"line_number":482,"context_line":"        # Retry setting the last context manager if we detect that a writer"},{"line_number":483,"context_line":"        # changed global DB state before we take the read lock."},{"line_number":484,"context_line":"        last_ctxt_mgr_desired \u003d None"},{"line_number":485,"context_line":"        while not last_ctxt_mgr_desired:"},{"line_number":486,"context_line":"            try:"},{"line_number":487,"context_line":"                with self._cell_lock.read_lock():"},{"line_number":488,"context_line":"                    if self._last_ctxt_mgr !\u003d desired:"}],"source_content_type":"text/x-python","patch_set":1,"id":"4bb150c4_5e2d61fc","line":485,"range":{"start_line":485,"start_character":8,"end_line":485,"end_character":13},"updated":"2022-03-23 19:04:38.000000000","message":"It seems a little concerning to make this a hard and infinite loop. If we starve out any other threads or fail to eventually give up we could be introducing a deadlock here. I think two threads that hit this exactly right could be tossing the global state back and forth between their setting, checking, resetting, checking, etc.\n\nIs there any reason not to cap this at some reasonable retry value (perhaps maybe only try twice?) to avoid that? It also seems like we want some schedule-yielding delay in the retry, like a sleep(1) in the except handler to further give a chance for other things to progress before we retry the same thing again. If you use something like \"for retry in range(0, 3)\" then you could also sleep(retry) to increase the backoff from \"no wait but schedule\" to longer delays.","commit_id":"6ad153401ec18688cf9732adb1fffa6a3397b43e"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d8344b76c7c60783fbeaa19eb1828f89cbd0c61e","unresolved":false,"context_lines":[{"line_number":482,"context_line":"        # Retry setting the last context manager if we detect that a writer"},{"line_number":483,"context_line":"        # changed global DB state before we take the read lock."},{"line_number":484,"context_line":"        last_ctxt_mgr_desired \u003d None"},{"line_number":485,"context_line":"        while not last_ctxt_mgr_desired:"},{"line_number":486,"context_line":"            try:"},{"line_number":487,"context_line":"                with self._cell_lock.read_lock():"},{"line_number":488,"context_line":"                    if self._last_ctxt_mgr !\u003d desired:"}],"source_content_type":"text/x-python","patch_set":1,"id":"45d38cf2_b6a40ca3","line":485,"range":{"start_line":485,"start_character":8,"end_line":485,"end_character":13},"in_reply_to":"3862476c_dbd18a94","updated":"2022-03-24 18:47:09.000000000","message":"Done","commit_id":"6ad153401ec18688cf9732adb1fffa6a3397b43e"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"939018e3250fca024d324e8c7c256722624686d4","unresolved":true,"context_lines":[{"line_number":482,"context_line":"        # Retry setting the last context manager if we detect that a writer"},{"line_number":483,"context_line":"        # changed global DB state before we take the read lock."},{"line_number":484,"context_line":"        last_ctxt_mgr_desired \u003d None"},{"line_number":485,"context_line":"        while not last_ctxt_mgr_desired:"},{"line_number":486,"context_line":"            try:"},{"line_number":487,"context_line":"                with self._cell_lock.read_lock():"},{"line_number":488,"context_line":"                    if self._last_ctxt_mgr !\u003d desired:"}],"source_content_type":"text/x-python","patch_set":1,"id":"fdf544c7_cc5d2c77","line":485,"range":{"start_line":485,"start_character":8,"end_line":485,"end_character":13},"in_reply_to":"4bb150c4_5e2d61fc","updated":"2022-03-23 19:20:35.000000000","message":"Hm, OK. I had been thinking one would have to progress ahead of the other upon a retry but I think you are right that if the other retries at the \"right\" time it could go on again. And good point about adding a schedule-yield in here, I missed that. I\u0027ll change this to cap retries and do a sleep().","commit_id":"6ad153401ec18688cf9732adb1fffa6a3397b43e"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"020e2b6f1a5b9d342f55fd5d5e92aa90dad775f0","unresolved":true,"context_lines":[{"line_number":482,"context_line":"        # Retry setting the last context manager if we detect that a writer"},{"line_number":483,"context_line":"        # changed global DB state before we take the read lock."},{"line_number":484,"context_line":"        last_ctxt_mgr_desired \u003d None"},{"line_number":485,"context_line":"        while not last_ctxt_mgr_desired:"},{"line_number":486,"context_line":"            try:"},{"line_number":487,"context_line":"                with self._cell_lock.read_lock():"},{"line_number":488,"context_line":"                    if self._last_ctxt_mgr !\u003d desired:"}],"source_content_type":"text/x-python","patch_set":1,"id":"3862476c_dbd18a94","line":485,"range":{"start_line":485,"start_character":8,"end_line":485,"end_character":13},"in_reply_to":"fdf544c7_cc5d2c77","updated":"2022-03-23 19:23:55.000000000","message":"It\u0027s a classic dining philosophers problem where you have to do two things to proceed, along with your dining partner. Kinda has to be that way because we need the actual work to be reentrant (hence the reader lock) and probably the reason I initially punted until it actually became a problem. But yeah, in general, good to add something to break up a tight loop like this so that something changes.","commit_id":"6ad153401ec18688cf9732adb1fffa6a3397b43e"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"32b7c4ee4042bb17f943a5bd129df8007f81d196","unresolved":true,"context_lines":[{"line_number":497,"context_line":"                        # above until we land here with the cell we want."},{"line_number":498,"context_line":"                        raise RuntimeError("},{"line_number":499,"context_line":"                            \u0027Global DB state changed underneath us\u0027)"},{"line_number":500,"context_line":"                    last_ctxt_mgr_desired \u003d True"},{"line_number":501,"context_line":"                    try:"},{"line_number":502,"context_line":"                        with self._real_target_cell("},{"line_number":503,"context_line":"                            context, cell_mapping"}],"source_content_type":"text/x-python","patch_set":1,"id":"fdbd1dea_2e684e16","line":500,"range":{"start_line":500,"start_character":20,"end_line":500,"end_character":48},"updated":"2022-03-23 19:04:38.000000000","message":"This is okay I guess, but why not just put a break after L507? That further asserts and exposes the \"we expect to run this once unless interrupted\" behavior. We\u0027d never want to accidentally loop if we make it that far. If you convert this to for(i in retries) then the break will fit better too.","commit_id":"6ad153401ec18688cf9732adb1fffa6a3397b43e"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d8344b76c7c60783fbeaa19eb1828f89cbd0c61e","unresolved":false,"context_lines":[{"line_number":497,"context_line":"                        # above until we land here with the cell we want."},{"line_number":498,"context_line":"                        raise RuntimeError("},{"line_number":499,"context_line":"                            \u0027Global DB state changed underneath us\u0027)"},{"line_number":500,"context_line":"                    last_ctxt_mgr_desired \u003d True"},{"line_number":501,"context_line":"                    try:"},{"line_number":502,"context_line":"                        with self._real_target_cell("},{"line_number":503,"context_line":"                            context, cell_mapping"}],"source_content_type":"text/x-python","patch_set":1,"id":"a4ad8917_5ed41e79","line":500,"range":{"start_line":500,"start_character":20,"end_line":500,"end_character":48},"in_reply_to":"276d492b_38810d6d","updated":"2022-03-24 18:47:09.000000000","message":"Done","commit_id":"6ad153401ec18688cf9732adb1fffa6a3397b43e"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"e21a60f18ed30428f157af68a335d93d9de31075","unresolved":true,"context_lines":[{"line_number":497,"context_line":"                        # above until we land here with the cell we want."},{"line_number":498,"context_line":"                        raise RuntimeError("},{"line_number":499,"context_line":"                            \u0027Global DB state changed underneath us\u0027)"},{"line_number":500,"context_line":"                    last_ctxt_mgr_desired \u003d True"},{"line_number":501,"context_line":"                    try:"},{"line_number":502,"context_line":"                        with self._real_target_cell("},{"line_number":503,"context_line":"                            context, cell_mapping"}],"source_content_type":"text/x-python","patch_set":1,"id":"276d492b_38810d6d","line":500,"range":{"start_line":500,"start_character":20,"end_line":500,"end_character":48},"in_reply_to":"4ae4d21d_c4b3cf76","updated":"2022-03-23 19:28:43.000000000","message":"No I definitely agree it\u0027s better, I just didn\u0027t do it for some unknown reason ¯\\_(ツ)_/¯","commit_id":"6ad153401ec18688cf9732adb1fffa6a3397b43e"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"020e2b6f1a5b9d342f55fd5d5e92aa90dad775f0","unresolved":true,"context_lines":[{"line_number":497,"context_line":"                        # above until we land here with the cell we want."},{"line_number":498,"context_line":"                        raise RuntimeError("},{"line_number":499,"context_line":"                            \u0027Global DB state changed underneath us\u0027)"},{"line_number":500,"context_line":"                    last_ctxt_mgr_desired \u003d True"},{"line_number":501,"context_line":"                    try:"},{"line_number":502,"context_line":"                        with self._real_target_cell("},{"line_number":503,"context_line":"                            context, cell_mapping"}],"source_content_type":"text/x-python","patch_set":1,"id":"4ae4d21d_c4b3cf76","line":500,"range":{"start_line":500,"start_character":20,"end_line":500,"end_character":48},"in_reply_to":"f1bd5b2b_f42b834a","updated":"2022-03-23 19:23:55.000000000","message":"Ack, just seems a little cleaner to me. I dunno why I said \"this is okay I _guess_\" ... it\u0027s clearly okay as it is, just a little neater to use break I think :)","commit_id":"6ad153401ec18688cf9732adb1fffa6a3397b43e"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"939018e3250fca024d324e8c7c256722624686d4","unresolved":true,"context_lines":[{"line_number":497,"context_line":"                        # above until we land here with the cell we want."},{"line_number":498,"context_line":"                        raise RuntimeError("},{"line_number":499,"context_line":"                            \u0027Global DB state changed underneath us\u0027)"},{"line_number":500,"context_line":"                    last_ctxt_mgr_desired \u003d True"},{"line_number":501,"context_line":"                    try:"},{"line_number":502,"context_line":"                        with self._real_target_cell("},{"line_number":503,"context_line":"                            context, cell_mapping"}],"source_content_type":"text/x-python","patch_set":1,"id":"f1bd5b2b_f42b834a","line":500,"range":{"start_line":500,"start_character":20,"end_line":500,"end_character":48},"in_reply_to":"fdbd1dea_2e684e16","updated":"2022-03-23 19:20:35.000000000","message":"Hm, I don\u0027t know why I didn\u0027t do that. I guess I didn\u0027t think of it for some reason. Will change it.","commit_id":"6ad153401ec18688cf9732adb1fffa6a3397b43e"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"ca6660611cea385e183b2addf06b121223261694","unresolved":true,"context_lines":[{"line_number":506,"context_line":"                    except Exception as exc:"},{"line_number":507,"context_line":"                        raised_exc \u003d exc"},{"line_number":508,"context_line":"            except RuntimeError:"},{"line_number":509,"context_line":"                set_last_ctxt_mgr()"},{"line_number":510,"context_line":""},{"line_number":511,"context_line":"        with self._cell_lock.write_lock():"},{"line_number":512,"context_line":"            # Once we have returned from the context, we need"}],"source_content_type":"text/x-python","patch_set":1,"id":"df57dbed_2db36577","line":509,"updated":"2022-02-28 11:22:52.000000000","message":"OK, I think this is fixing the problem when the fixture detects that the target changed due to a racing thread. But I\u0027m not sure what will happen with the racing thread. That thread set a target that is now unset by this fixture.","commit_id":"6ad153401ec18688cf9732adb1fffa6a3397b43e"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"38653ea610beff4db30906e301777232f872f7ad","unresolved":true,"context_lines":[{"line_number":506,"context_line":"                    except Exception as exc:"},{"line_number":507,"context_line":"                        raised_exc \u003d exc"},{"line_number":508,"context_line":"            except RuntimeError:"},{"line_number":509,"context_line":"                set_last_ctxt_mgr()"},{"line_number":510,"context_line":""},{"line_number":511,"context_line":"        with self._cell_lock.write_lock():"},{"line_number":512,"context_line":"            # Once we have returned from the context, we need"}],"source_content_type":"text/x-python","patch_set":1,"id":"830e4be7_81b03c08","line":509,"in_reply_to":"1a2feee7_e574f79a","updated":"2022-03-04 11:06:46.000000000","message":"Ahh OK. Both thread going through this same code path. So both will apply the new retry. Cool. Thanks for the explanation.","commit_id":"6ad153401ec18688cf9732adb1fffa6a3397b43e"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d8344b76c7c60783fbeaa19eb1828f89cbd0c61e","unresolved":false,"context_lines":[{"line_number":506,"context_line":"                    except Exception as exc:"},{"line_number":507,"context_line":"                        raised_exc \u003d exc"},{"line_number":508,"context_line":"            except RuntimeError:"},{"line_number":509,"context_line":"                set_last_ctxt_mgr()"},{"line_number":510,"context_line":""},{"line_number":511,"context_line":"        with self._cell_lock.write_lock():"},{"line_number":512,"context_line":"            # Once we have returned from the context, we need"}],"source_content_type":"text/x-python","patch_set":1,"id":"942bdc99_f1c5488b","line":509,"in_reply_to":"830e4be7_81b03c08","updated":"2022-03-24 18:47:09.000000000","message":"Ack","commit_id":"6ad153401ec18688cf9732adb1fffa6a3397b43e"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"f46a9912be980469c4ff720555527e54943f2c68","unresolved":true,"context_lines":[{"line_number":506,"context_line":"                    except Exception as exc:"},{"line_number":507,"context_line":"                        raised_exc \u003d exc"},{"line_number":508,"context_line":"            except RuntimeError:"},{"line_number":509,"context_line":"                set_last_ctxt_mgr()"},{"line_number":510,"context_line":""},{"line_number":511,"context_line":"        with self._cell_lock.write_lock():"},{"line_number":512,"context_line":"            # Once we have returned from the context, we need"}],"source_content_type":"text/x-python","patch_set":1,"id":"1a2feee7_e574f79a","line":509,"in_reply_to":"df57dbed_2db36577","updated":"2022-03-04 04:06:06.000000000","message":"Hm... so each thread has a \u0027desired\u0027 cell it wants to target and if Thread A sets to cell1 and then Thread B sets to cell0 \"at the same time\" then Thread A will detect the change at L488 and set it back to cell1 and try again. Now Thread B resumes and enters locked section L487 and since its \u0027desired\u0027 cell is cell0 and Thread A had set _last_ctxt_mgr \u003d cell1, it will detect the change at L488 and then retry and set it back to cell0 and then check _last_ctxt_mgr !\u003d \u0027desired\u0027 again and when it matches, proceed to the DB call.\n\nIt seems like racing threads will always retry and end up getting their \u0027desired\u0027 cell. Let me know if you see any gaps that I missed.","commit_id":"6ad153401ec18688cf9732adb1fffa6a3397b43e"}]}