feat(daemon): async interaction with children #2948
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2948 +/- ##
==========================================
- Coverage 89.10% 88.96% -0.14%
==========================================
Files 138 138
Lines 9607 9607
==========================================
- Hits 8560 8547 -13
- Misses 1047 1060 +13
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
The |
Latency summaryCurrent PR yields:
Breakdown
Backed by latency-tracking. Further commits will update this comment. |
Fixed now |
| _update_default_args() | ||
| pathlib.Path(__root_workspace__).mkdir(parents=True, exist_ok=True) | ||
| if not jinad_args.no_fluentd: | ||
| Thread(target=_start_fluentd, daemon=True).start() | ||
| _start_consumer() | ||
| _start_uvicorn(app=_get_app(mode=jinad_args.mode)) | ||
|
|
||
|
|
||
| def teardown(): |
There was a problem hiding this comment.
Shouldn't we stop the consumer thread and uvicorn here?
There was a problem hiding this comment.
Consumer thread is a daemon thread, so no need to close it manually.
Uvicorn handles its own shutdown logic in serve itself for different signal handlers.
| async with aiohttp.request( | ||
| method='PUT', url=f'{uri}/{self._kind}', params=params | ||
| ) as response: | ||
| response_json = await response.json() |
There was a problem hiding this comment.
What happens if there is some issue with the json deserialization? Same goes for other cases. Does this work then here or throw an exception?
There was a problem hiding this comment.
That gets caught in containers.py exception handler and raised to the invoker. But our exception sending logic to the user is poor now. It needs quite some work and cannot be done in this PR
|
|
||
|
|
||
| @pytest.yield_fixture(scope="session") | ||
| def event_loop(request): |
There was a problem hiding this comment.
Cant we use the default loop fixture? If so (I think the scope is different), the comment below seems to be wrong as the scope here session and not 'for each test'
There was a problem hiding this comment.
You're right. The comment is wrong. It needs to be a session, else we need to handle event loop (start and closure) logic in aiohttp client session, which is not required in the actual code (as uvicorn takes care of it), and needed only in tests.
I had to spent >2hrs just to fix this :)
839b7af to
953c320
Compare
953c320 to
abc5d88
Compare
Fixes #2746