Skip to content

fix: improve lifespan handling in asgi.py#148

Merged
ryanking13 merged 5 commits into
mainfrom
gyeongjae/asgi-refine
Jul 9, 2026
Merged

fix: improve lifespan handling in asgi.py#148
ryanking13 merged 5 commits into
mainfrom
gyeongjae/asgi-refine

Conversation

@ryanking13

@ryanking13 ryanking13 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

split out from #147. It only includes asgi.py lifespan + null body handling changes with corresponding tests without django-specific tests.

@ask-bonk

ask-bonk Bot commented Jul 7, 2026

Copy link
Copy Markdown

Now let me carefully review the code changes:

  1. start_application rewrite (asgi.py:78-159) — The old implementation used iter([..., Future()]) which would block forever if the app didn't support lifespan. The new implementation uses a Queue for receive and properly handles:

    • lifespan.startup.complete / lifespan.startup.failed
    • lifespan.shutdown.complete / lifespan.shutdown.failed
    • Apps that raise exceptions (treated as no-lifespan)
    • Apps that return without acking (treated as no-lifespan)
  2. Null-body statuses (asgi.py:236-243) — New NULL_BODY_STATUSES set correctly handles 101/103/204/205/304 per Fetch spec.

  3. Defensive .get() for optional keys (asgi.py:210, 213)headers and body now use .get() with defaults.

  4. Tests — Comprehensive coverage for all new behaviors.

Let me check for potential issues:

  • Line 94 in asgi.py: shutdown() checks startup.done() and not startup.result(). If startup resolved with an exception (startup.failed), calling startup.result() would re-raise the exception. However, in start_application, line 156 does supported = await startup which would raise the exception there, so shutdown() would never be called in that case because the exception propagates out of fetch(). This is safe.

  • Lines 143-153: In the except block of run_lifespan, if startup is already done (meaning startup succeeded), the exception is logged and shutdown is completed. This is a reasonable approach — the request was already served, so the exception during shutdown shouldn't crash the response.

  • no_lifespan_shutdown vs shutdown: When lifespan is unsupported, returning no_lifespan_shutdown (a no-op) is correct since there's nothing to shut down.

  • NULL_BODY_STATUSES includes 101 and 103: 101 (Switching Protocols) and 103 (Early Hints) are included. In the test, only 204, 205, and 304 are tested. Status 101 and 103 are more specialized and harder to test in this context, but having them in the set is still correct per the Fetch spec.

  • Potential issue with shutdown on line 94: The shutdown() function checks if startup.done() and not startup.result(). When startup.result() is True, not True is False, so it proceeds to put the shutdown message. When it's False, not False is True, so it returns early. This logic is correct.

  • Race condition consideration: run_lifespan() is launched via run_in_background() which uses ensure_future. Then await startup blocks until the lifespan task resolves the future. The Queue ensures that messages are delivered in order. This looks correct.

Let me verify one more thing — whether the shutdown() function properly awaits shutdown_complete and whether there's a potential for it to hang if the app's lifespan handler silently returns after receiving the shutdown message without sending shutdown.complete:

Looking at run_lifespan() lines 140-142: if the app returns without setting shutdown_complete, the code checks elif not shutdown_complete.done(): shutdown_complete.set_result(None). This handles the case where the app returns from the lifespan scope without explicitly sending shutdown.complete — it resolves the future so shutdown() won't hang. Good.

The code looks solid. The only thing I notice that's worth flagging:

There is one subtle issue — after a successful startup, if the lifespan coroutine crashes during the shutdown phase, the exception handler at line 151 logs it and resolves shutdown_complete. But shutdown() at line 97 does await shutdown_complete — since set_result(None) is called, this will resolve cleanly rather than propagating the error. This is inconsistent with the lifespan.shutdown.failed path (line 120-122) which uses set_exception and does propagate. The comment at line 149 says "a shutdown-phase error can't affect the already-served request" which is a reasonable argument, but the inconsistency between an app that sends shutdown.failed vs an app that raises an exception during shutdown could be surprising.

However, this is a reasonable design choice — the ASGI spec says an exception during the lifespan scope is different from explicitly sending a failure message. This is arguably correct behavior.

Overall, this is a well-structured change with good test coverage.

LGTM

github run

Comment thread packages/cli/tests/workerd-test/asgi/worker.py

@hoodmane hoodmane left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Comment thread packages/cli/tests/workerd-test/asgi/tests/test_asgi.py Outdated
@ryanking13 ryanking13 merged commit 7c9adfc into main Jul 9, 2026
16 checks passed
@ryanking13 ryanking13 deleted the gyeongjae/asgi-refine branch July 9, 2026 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants