-
Notifications
You must be signed in to change notification settings - Fork 478
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HITL: Add HTTP availability server #1794
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments for reviewers
@@ -2,6 +2,10 @@ | |||
|
|||
habitat_hitl: | |||
window: ~ | |||
networking: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments in hitl_defaults.yaml.
threading.Event() if multiprocessing_config.use_dummy else None | ||
) | ||
# we don't support dummy multiprocessing at this time | ||
assert not multiprocessing_config.use_dummy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not worth the complexity of maintaining the dummy-multiprocessing version of all this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just remove it, and introduce it again if we need it?
start_server = websockets.serve( | ||
network_mgr_lambda, "0.0.0.0", 8888, ssl=ssl_context | ||
|
||
async def networking_main_async(interprocess_record): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The big refactor here is that all our networking logic is now implemented with async/await
. Reviewers should take some time to make sure they understand how networking_main_async
and networking_main
work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I tested the networking server, and it works as expected.
threading.Event() if multiprocessing_config.use_dummy else None | ||
) | ||
# we don't support dummy multiprocessing at this time | ||
assert not multiprocessing_config.use_dummy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just remove it, and introduce it again if we need it?
# Handle SIGTERM. We should get this signal when we do networking_process.terminate(). See terminate_networking_process. | ||
stop: asyncio.Future = asyncio.Future() | ||
loop = asyncio.get_event_loop() | ||
loop.add_signal_handler(signal.SIGTERM, stop.set_result, None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
* add http availability server; refactor of networking process main loop * add HTML stub client * fixups after rebasing * documentation polish
* add http availability server; refactor of networking process main loop * add HTML stub client * fixups after rebasing * documentation polish
Motivation and Context
How Has This Been Tested
Local testing and AWS testing with HTML stub client.
Types of changes
Checklist