bug: idsse-795: PublishConfirm setup timeout #67
Conversation
There was a problem hiding this comment.
I think a timeout for setup is a good idea. See the linear task for a post that might help with logs from threads. I also put on comment that I think would be good for original bug, it follows the logic in the RPC class. Also see PublisherSync for the code I added to re-connect, though you might be able it improve on the implementation.
| if callback is not None: | ||
| self._on_ready_callback = callback # to be invoked after all pika setup is done | ||
| if is_ready is not None: | ||
| self._is_ready_future = is_ready # to be invoked after all pika setup is done | ||
| self._thread.start() |
There was a problem hiding this comment.
I think you should put an if self._thread.is_alive around this, but that is more for the original problem not what is covered in this PR
There was a problem hiding this comment.
I went back on forth on this, and either way your comment points to how messy and hard-to-read this class has gotten.
I put an is_alive() check in the public method start(), which in turn calls this private _start() method. However, the methods aren't identical: public start doesn't accept a callback/Future argument, which the private _start does. And _wait_for_channel_to_be_ready calls the private start, not the public one.
Should I just combine them to be one consistent function? Or is there a better organization?
There was a problem hiding this comment.
I would put the 'is_alive' check right before the thread.start(), that means it is always checked. As of right now I don't see the need for a public and private start, so I'd get rid of the private, which is how rpc_client.py is written.
|
|
||
| Returns: | ||
| bool: True if channel is ready. False if timed out waiting for RabbitMQ to connect | ||
| """ | ||
|
|
||
| # validate that PublishConfirm thread has been set up and connected to RabbitMQ | ||
| logger.info('DEBUG _wait_for_channel_to_be_ready state') |
There was a problem hiding this comment.
These look like debugs not info logs
Linear Issue
IDSSE-795
Changes
_wait_for_channel_to_be_ready(). Default is 6 secondsExplanation
My current theory is that PublishConfirm hanging/deadlocking bug was due to the child Thread failing, for some reason, to connect to RabbitMQ and establish its exchange/queue... but because it's not the main thread, no logs were appearing in the console/stdout to help us troubleshoot. At least this is what I saw locally, and adding debug breakpoints helped me find that my exchange/queue parameters did not match the existing exchanges and queues on the RabbitMQ server.
These changes may not resolve any issues, but it will surface them more clearly, by creating a Python
Futurewhich the RabbitMQ setup callbacks will either resolve to True if everything worked, or an exception if RabbitMQ throws one, or timeout after 6 seconds.