-
Notifications
You must be signed in to change notification settings - Fork 4
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
fix(engine): fail on autostart STREAM or SERVICE #122
fix(engine): fail on autostart STREAM or SERVICE #122
Conversation
assert event_info.read_stream | ||
logger.info( | ||
__name__, f"STREAM start event_name={event_name} read_stream={event_info.read_stream.name}") | ||
asyncio.create_task(app_engine.read_stream(event_name=event_name)) |
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.
Before introducing so many changes an couple stream start to aiohttp startup hook, can we diagnose what was causing the streams not to start using regular asyncio.createtask? Was this code reachable and actually starting the streams? Anything in the logs?
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 issue looks like run an asyncio.create_task(..) inside of the loop.run_until_complete() call won't start properly the task. Running the simple_example app you can observe no streams or services are listening after startup and no logs about Starting reading stream...
, you need to start each one with the management API, and then works fine.
Reading this topic on aiohttp docs I found the strategy I propose in the pr, it shows how to add create_task to a running web.Application.
The current commit now starts the streams and services if you use --start-streams on the startup arguments.
And logs now show the usual behavior Starting reading stream...
.
@@ -489,7 +489,7 @@ async def start_test_server( | |||
mock_app_config, mock_plugin_config, streams=None): # noqa: F811 | |||
await start_server(mock_app_config.server) | |||
await start_app(mock_plugin_config) | |||
await start_app(mock_app_config, start_streams=streams) | |||
await start_app(mock_app_config) |
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.
Before introducing a fix we need to create a test that reproduces the issue.
Reimplemented in #123 removing calls to loop.run_until_complete |
close #121