)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":2813,"name":"Mehdi Abaakouk","email":"sileht@sileht.net","username":"sileht"},"change_message_id":"75cf6b953d11795716bbd6fade1838dbd24a4585","unresolved":false,"context_lines":[{"line_number":15,"context_line":"initialisation in start() before starting a worker thread and"},{"line_number":16,"context_line":"returning control to the caller. That is, the caller can be sure that"},{"line_number":17,"context_line":"this initialisation has occurred when control is returned. However, in"},{"line_number":18,"context_line":"the blocking case, control is never returned. We currently work round"},{"line_number":19,"context_line":"this by setting self._running to True before executing executor.start,"},{"line_number":20,"context_line":"and by not doing any locking whatsoever in MessageHandlingServer."},{"line_number":21,"context_line":"However, this current means there is a race whereby executor.stop()"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"3a29b11f_c90ccc96","line":18,"updated":"2015-10-20 10:08:41.000000000","message":"I don\u0027t think we have a workaround here, the place of self._running doesn\u0027t change anything because the whole block in protected by the condition.\n\nI just think no test show up the issue you describe and the blocking executor is currently broken, no matter what the start/stop order.\n\nAnyways the fix itself looks good.","commit_id":"7537601fc664b61b0e6e6718a9e853927429b891"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"3879e830ea4cdf7eabeca1e45a475af9e451e477","unresolved":false,"context_lines":[{"line_number":15,"context_line":"initialisation in start() before starting a worker thread and"},{"line_number":16,"context_line":"returning control to the caller. That is, the caller can be sure that"},{"line_number":17,"context_line":"this initialisation has occurred when control is returned. However, in"},{"line_number":18,"context_line":"the blocking case, control is never returned. We currently work round"},{"line_number":19,"context_line":"this by setting self._running to True before executing executor.start,"},{"line_number":20,"context_line":"and by not doing any locking whatsoever in MessageHandlingServer."},{"line_number":21,"context_line":"However, this current means there is a race whereby executor.stop()"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"3a29b11f_7fa446a5","line":18,"in_reply_to":"3a29b11f_c90ccc96","updated":"2015-10-20 10:30:33.000000000","message":"Yeah, this code is really messy. It\u0027s also complicated by the fact that _state_cond is sometimes a noop. I actually wrote a test which I intended to include in this patch which starts and stops a blocking server, expecting it to hang. I had a serious WTF moment until I remembered that what appeared to be locking was actually not.\n\nI chucked the test in with the next patch anyway, btw. The reason is that the next patch unconditionally enables locking, which means that it\u0027s important to check that the blocking executor can still be started and stopped in this case.","commit_id":"7537601fc664b61b0e6e6718a9e853927429b891"}],"oslo_messaging/_executors/impl_blocking.py":[{"author":{"_account_id":1297,"name":"Joshua Harlow","email":"jxharlow@godaddy.com","username":"harlowja"},"change_message_id":"ab84b1da583829a9066a3e6a90c296f7ee675e4e","unresolved":false,"context_lines":[{"line_number":40,"context_line":"            self._running_cond.notify_all()"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":"    def join(self, timeout\u003dNone):"},{"line_number":43,"context_line":"        # FIXME: Use monotonic time when this bug is fixed:"},{"line_number":44,"context_line":"        # https://github.com/atdt/monotonic/pull/12"},{"line_number":45,"context_line":"        if timeout is not None:"},{"line_number":46,"context_line":"            limit \u003d time.time() + timeout"}],"source_content_type":"text/x-python","patch_set":1,"id":"3a29b11f_0ab7363a","line":43,"updated":"2015-10-19 17:35:25.000000000","message":"Good find. Hopefully that gets fixed soon.","commit_id":"de4bcdbfaa3e9ca45fde1669e7f56753135eace5"},{"author":{"_account_id":1297,"name":"Joshua Harlow","email":"jxharlow@godaddy.com","username":"harlowja"},"change_message_id":"4ac06ff79eb673d6839f567a75ce7d9efc08d2df","unresolved":false,"context_lines":[{"line_number":40,"context_line":"            self._running_cond.notify_all()"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":"    def join(self, timeout\u003dNone):"},{"line_number":43,"context_line":"        # FIXME: Use monotonic time when this bug is fixed:"},{"line_number":44,"context_line":"        # https://github.com/atdt/monotonic/pull/12"},{"line_number":45,"context_line":"        if timeout is not None:"},{"line_number":46,"context_line":"            limit \u003d time.time() + timeout"}],"source_content_type":"text/x-python","patch_set":1,"id":"3a29b11f_59d73c23","line":43,"updated":"2015-10-19 23:49:39.000000000","message":"U can also just use:\n\nhttp://docs.openstack.org/developer/oslo.utils/api/timeutils.html#oslo_utils.timeutils.StopWatch\n\nAlthough its using the same monotonic library (nobody has reported any issues though, which is interesting on its own),\n\nUsage can be done like:\n\nhttps://github.com/openstack/oslo.messaging/blob/master/oslo_messaging/_executors/impl_pooledexecutor.py#L121\n\nOr something similar to that...","commit_id":"de4bcdbfaa3e9ca45fde1669e7f56753135eace5"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"55b8b210b33f720cc1d0d8200212b83470638f44","unresolved":false,"context_lines":[{"line_number":40,"context_line":"            self._running_cond.notify_all()"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":"    def join(self, timeout\u003dNone):"},{"line_number":43,"context_line":"        # FIXME: Use monotonic time when this bug is fixed:"},{"line_number":44,"context_line":"        # https://github.com/atdt/monotonic/pull/12"},{"line_number":45,"context_line":"        if timeout is not None:"},{"line_number":46,"context_line":"            limit \u003d time.time() + timeout"}],"source_content_type":"text/x-python","patch_set":1,"id":"3a29b11f_6396fa4d","line":43,"in_reply_to":"3a29b11f_59d73c23","updated":"2015-10-20 08:10:09.000000000","message":"Hmm... I guess I could just use it now in the knowledge that it\u0027s likely to be fixed fairly soon.","commit_id":"de4bcdbfaa3e9ca45fde1669e7f56753135eace5"},{"author":{"_account_id":1297,"name":"Joshua Harlow","email":"jxharlow@godaddy.com","username":"harlowja"},"change_message_id":"ab84b1da583829a9066a3e6a90c296f7ee675e4e","unresolved":false,"context_lines":[{"line_number":57,"context_line":"    def is_alive(self):"},{"line_number":58,"context_line":"        return self._running"},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"    def execute(self):"},{"line_number":61,"context_line":"        assert(self.running)"},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":1,"id":"3a29b11f_0ae5f635","line":60,"updated":"2015-10-19 17:35:25.000000000","message":"Same question as below.","commit_id":"de4bcdbfaa3e9ca45fde1669e7f56753135eace5"},{"author":{"_account_id":1297,"name":"Joshua Harlow","email":"jxharlow@godaddy.com","username":"harlowja"},"change_message_id":"ab84b1da583829a9066a3e6a90c296f7ee675e4e","unresolved":false,"context_lines":[{"line_number":59,"context_line":""},{"line_number":60,"context_line":"    def execute(self):"},{"line_number":61,"context_line":"        assert(self.running)"},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"        try:"},{"line_number":64,"context_line":"            self._target()"},{"line_number":65,"context_line":"        finally:"}],"source_content_type":"text/x-python","patch_set":1,"id":"3a29b11f_6a6eeaca","line":62,"updated":"2015-10-19 17:35:25.000000000","message":"Asserts can be compiled out, do u want to throw a runtime error or something else that can\u0027t be compiled out?\n\nhttps://docs.python.org/3/reference/simple_stmts.html#assert\n\n\"The current code generator emits no code for an assert statement when optimization is requested at compile time.\"","commit_id":"de4bcdbfaa3e9ca45fde1669e7f56753135eace5"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"55b8b210b33f720cc1d0d8200212b83470638f44","unresolved":false,"context_lines":[{"line_number":59,"context_line":""},{"line_number":60,"context_line":"    def execute(self):"},{"line_number":61,"context_line":"        assert(self.running)"},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"        try:"},{"line_number":64,"context_line":"            self._target()"},{"line_number":65,"context_line":"        finally:"}],"source_content_type":"text/x-python","patch_set":1,"id":"3a29b11f_83d9d685","line":62,"in_reply_to":"3a29b11f_6a6eeaca","updated":"2015-10-20 08:10:09.000000000","message":"I don\u0027t feel strongly about it, tbh. Hopefully we\u0027re most likely to trip over asserts in testing, where they\u0027re more likely to be enabled.\n\nIf we have an existing coding standard I\u0027m happy to use it. If not, I\u0027m happy to leave it as it.","commit_id":"de4bcdbfaa3e9ca45fde1669e7f56753135eace5"},{"author":{"_account_id":2813,"name":"Mehdi Abaakouk","email":"sileht@sileht.net","username":"sileht"},"change_message_id":"75cf6b953d11795716bbd6fade1838dbd24a4585","unresolved":false,"context_lines":[{"line_number":33,"context_line":"        self._running_cond \u003d threading.Condition()"},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"    def start(self):"},{"line_number":36,"context_line":"        assert(not self._running)"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"        with self._running_cond:"},{"line_number":39,"context_line":"            self.running \u003d True"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a29b11f_e429ad38","line":36,"updated":"2015-10-20 10:08:41.000000000","message":"We should not use assert in running code.\nWe should remove it or raise RuntimeError(...) to ensure the lib always have the same behavior and not depend of how the interpreter is started. (And I\u0027m really fine with RuntimeError :) )","commit_id":"7537601fc664b61b0e6e6718a9e853927429b891"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"3879e830ea4cdf7eabeca1e45a475af9e451e477","unresolved":false,"context_lines":[{"line_number":33,"context_line":"        self._running_cond \u003d threading.Condition()"},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"    def start(self):"},{"line_number":36,"context_line":"        assert(not self._running)"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"        with self._running_cond:"},{"line_number":39,"context_line":"            self.running \u003d True"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a29b11f_1f62f2ad","line":36,"in_reply_to":"3a29b11f_e429ad38","updated":"2015-10-20 10:30:33.000000000","message":"harlowja mentioned this, too. As I said, I don\u0027t feel strongly about it. If we have an existing coding standard I\u0027m happy to follow it. My (weak) preference is to leave it as is, as this seems to me to be what assert was written for, including the ability to turn it off if the user wants to.","commit_id":"7537601fc664b61b0e6e6718a9e853927429b891"}]}