henry3260 commented on PR #65587: URL: https://github.com/apache/airflow/pull/65587#issuecomment-4433318917
> > > How about adding `lifespan` to ensure `SUPERVISOR_COMMS` will be initialized for in-process Execution API? > > > > > > I think lifespan is the wrong scope for this. It initializes app-scoped state once at startup, but the issue here is request-scoped we need to isolate an in-process Execution API request from the outer client context. Initializing `SUPERVISOR_COMMS `in lifespan would make that client marker app-wide and could leak it into server-side request handling wdyt? > > For `dag.test()` command, it will use `InProcessTestSupervisor` under the hook, which means all the request will be processed in process. I'm not sure will there be race condition for `override_process_context` (when the `_PROCESS_CONTEXT_OVERRIDE.reset` teardown). > > IMO, it's no harm to set the lifespan for `InProcessTestSupervisor` only app (override the `app` instance in `_api_client` factory method) I understand your concern! Since `ContextVar` is independent per thread task, there shouldn't be any race conditions during the reset. That being said, I completely agree with your idea to minimize the influence area. I will update the PR to apply the override specifically within `InProcessTestSupervisor`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
