Conversation
…mestamps to clients
jameshiebert
left a comment
jameshiebert
left a comment
There was a problem hiding this comment.
Looks pretty straightforward and easy to understand which is good! I'm glad that the solution didn't turn out to be much more complicated than this. I have a few minor concerns and bigger concerns about the blocking I/O, but I also don't want that to derail us for too long. If you can try to do it with async, that'd be good, but if it takes took long to figure it out, I'm willing to accept this way.
| listener._listener._socket.settimeout(0.5) # Ensure that listener is not always waiting for connection in case there is no Client | ||
| while not event.is_set(): | ||
| try: | ||
| with listener.accept() as conn: |
There was a problem hiding this comment.
I believe that this uses blocking I/O which is non-ideal. Most of the time, it's going to be sitting there doing nothing, blocking execution of anything else. Now, maybe that doesn't matter if it's running in a thread? I'm not sure. But if it's possible to write this using cooperative multitasking (like with Python's await keyword), that might be better. See: https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.run_in_executor
| # -------------------------------------------------------------------- # | ||
| # create Listener to send timestamp information to connecting Client | ||
| def send_timestamp(timestamp, event): | ||
| address = ("0.0.0.0", 5005) |
There was a problem hiding this comment.
Would we have any concerns about potential DOS attacks on this?
There was a problem hiding this comment.
Would a possible way to account for this be to create a docker network with just the osprey and osprey-flask-app containers? Or check that the address returned in listener.accept() is the IP address of the osprey-flask-app container?
There was a problem hiding this comment.
Yes, having the two containers on their own network would mitigate the risk of a DOS attack. Aside from that, it's generally a good practice to make the listening address and port to be run time parameters (probably with address defaulting to localhost/127.0.0.1), so that admins have some flexibility in how they deploy the service.
There was a problem hiding this comment.
I put the two containers in their own network.
| "matplotlib >= 1.3.1", | ||
| "pandas >= 1.0", | ||
| ], | ||
| install_requires=[req.strip() for req in open("requirements.txt")], |
There was a problem hiding this comment.
We're trying to move away from using setup.py in favour of pyproject.toml. A precondition of that is not having dynamic content in setup.py (ask @rod-glover about his most recent work in pycds). As such, it would be better to not add any dynamic content here.
| event.set() | ||
| listener_thread.join() |
There was a problem hiding this comment.
Should these be executed in the finally block? I.e. I think that we want them to run always, and as written here, they won't if some exception is raised by convolution_run. Speaking of which... why is there no except block corresponding to the try. Will this code will silently fail?
…bility in finally block
jameshiebert
left a comment
jameshiebert
left a comment
There was a problem hiding this comment.
Looks much better. Just a couple of things to tidy up :)
| # ---------------------------------------------------------------- # | ||
|
|
||
| except BaseException as e: | ||
| log.error(e, exc_info=True) |
There was a problem hiding this comment.
You could probably use log.exception here.
| log.error(e, exc_info=True) | ||
| raise(e) | ||
| finally: | ||
| if event: |
| gen_uh_final(outlets, dom_data, config_dict, directories) | ||
| # ---------------------------------------------------------------- # | ||
|
|
||
| except BaseException as e: |
There was a problem hiding this comment.
Again, I think that this is essentially a no-op. Can you explain the value-add here?
…rvic-daccs into i17-add-listener
|
This PR has been updated to get the listener port as a configuration option instead of an environment variable. This is because |
2 participants
This PR resolves #17. At the start of the convolution process, a thread is created to instantiate a Listener that sends the timestamp being read during the process to connecting clients. This thread is terminated after all the timestamps have been read. This functionality is primarily for assisting with the progress bar for the
osprey-flask-app.