Non blocking reads from process output #105
Conversation
| result = [] | ||
| try: | ||
| while True: | ||
| line = self.queue.get_nowait() |
There was a problem hiding this comment.
From: https://docs.python.org/3/library/queue.html#queue.Queue.get_nowait
"""
exception queue.Empty
Exception raised when non-blocking get() (or get_nowait()) is called on a Queue object which is empty.
"""
Seems to indicate that this call could raise an exception... do you want to catch and ignore here or annotate your docstring to indicate this could be raised?
There was a problem hiding this comment.
Just making sure, does the except block below already do this?
|
|
||
| def __init__(self, stream): | ||
| self.stream = stream | ||
| self.queue = Queue() |
There was a problem hiding this comment.
Unbounded Queue leaves you vulnerable to OOM issues. Would it be better to limit and throw an exception when out of bounds, or shed oldest entries?
There was a problem hiding this comment.
Good point, not sure how much of an issue it is but I will make a note to revisit this. For context, the size of the streams is much smaller than the output data, which is most likely to cause OOM issues.
Purpose
The api call that launches a simulation works by creating a process with
Popenand returning a json response that includes stdout/stderr. This content is updated each time the status endpoint is called by reading from the standard streams. Problem is the current way it's done blocks until the process is complete, which is typically a long time and not a good experience.What it does
Makes it so the http call returns immediately, by moving the blocking call to a separate thread, and using a thread safe queue to buffer output between that and the request thread. This way any call to either launch or check on a simulation will return the stdout/stderr even if the simulation is still running.
Testing
I had to fix the tests after refactoring, which ended up being good since now we are not using any fakes, so the tests are running the actual code. Since concurrency is involved, it's not totally clear how reliable this is, but the tests are simple enough that it seems to be stable.
Time to review
10-30 mins