fix(handler): wait for context.succeed when handler returns undefined#785
Closed
zarirhamza wants to merge 1 commit into
Closed
fix(handler): wait for context.succeed when handler returns undefined#785zarirhamza wants to merge 1 commit into
zarirhamza wants to merge 1 commit into
Conversation
Restore the pre-v11.126.0 semantics for the `(event, context) => { ... }` /
`length < 3` / no-return handler shape: when `asyncProm === undefined`, fall
back to `callbackProm` instead of resolving immediately with `undefined`.
PR #661 (v11.126.0) introduced an "eager-resolve on undefined" branch intended
to fix TypeScript-transpiled async handlers whose `return` was lost during
bundling. PR #665 (v12.127.0) refactored that branch and PR #683 (v12.130.0)
added a `looksLikeArtifact` heuristic for handlers that *return* a Server /
EventEmitter, but the `asyncProm === undefined` path was never re-guarded. As
a result every layer from v11.126.0 onwards silently truncates handlers that
return nothing and finish via `context.succeed` / `context.done` /
`context.fail`, most notably `aws-serverless-express@3`'s
`proxy(server, event, context)` with the default `CONTEXT_SUCCEED` resolution
mode.
Symptom: handler returns `undefined` synchronously, wrapper resolves
immediately with `undefined`, the Lambda runtime ships `undefined` back to the
caller and freezes the worker before any deferred work writes to stdout, so
nothing is flushed to CloudWatch.
Tests:
- Replace the test added by PR #661 that codified the regressive "return
undefined -> resolve immediately" behavior.
- Add four regression tests covering the actual contract: a handler with
`length < 3` and no return must wait for `context.succeed` / `context.done` /
`context.fail`, including a reproducer mirroring the aws-serverless-express
`proxy()` shape.
Also tighten `looksLikeArtifact` to require `asyncProm !== null` (the previous
check accepted `null`, which would have thrown on the subsequent `instanceof`
and property-access reads).
a37ca3a to
80e5f09
Compare
Contributor
Author
|
Superseded by #786 (rebranched off a non-customer-named branch). Closing this one. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Restore the pre-
v11.126.0semantics for the(event, context) => { ... }handler shape (no callback parameter, no explicit return). When the handler returnsundefined, the wrapper now waits forcontext.succeed/context.done/context.fail/callbackinstead of resolving the wrapping promise immediately withundefined.This fixes a regression for handlers using
aws-serverless-express@3'sproxy(server, event, context)pattern (defaultCONTEXT_SUCCEEDresolution mode) and any other "fire-and-forget then signal viacontext.*" handler shape.Root cause
#661 (
v11.126.0, "Fix Typescript Timeouts when Lambda handler returns undefined") added an eager-resolve branch:#665 (
v12.127.0) refactored this and #683 (v12.130.0) added alooksLikeArtifactheuristic that keeps waiting when the handler returns aServer/EventEmitter. However, the artifact heuristic gates onasyncProm !== undefined, so when the handler returns nothing it still falls through toPromise.resolve(undefined)— i.e. #683 never covered the no-return case.A handler that returns
undefinedsynchronously is indistinguishable from a handler that's mid-flight and intends to complete viacontext.succeed/context.done/context.fail. Eager-resolving onundefinedtruncates that work: the Lambda runtime shipsundefinedback to the caller and freezes the worker before pending stdout writes flush to CloudWatch.A representative handler shape that hits this:
handler.length === 2, no return, soasyncProm === undefined→ previously hit the eager-resolve branch and resolved before Express finished.Fix
Additional small hardening: tighten
looksLikeArtifactto requireasyncProm !== null(the prior check acceptednulland would then runinstanceof EventEmitter/ property reads on it).Why this is safe to revert
v11.125.0. Reverting restores a long-stable contract.context.succeed/done/failis called, or the returned promise settles". It is not "the function ends when the synchronous body returns". (fix): Fix Typescript Timeouts when Lambda handler returns undefined #661's branch silently changed that contract.undefinedand does nothing else is now left waiting oncallbackProm, which is correct: it will hit the configured Lambda function timeout. That is the expected behavior — a function that never signals completion is a user bug, and conflating it with "the handler is done" was the wrong fix.Notes for users still hitting timeouts with TS-bundled async handlers
The original problem #661 set out to solve (TS compilation losing the implicit return of an async handler) is better addressed at the user level: ensure your transpilation target supports native
async(Node ≥ 12), orreturnthe work explicitly. The wrapper should not silently truncate execution to compensate.Tests
The four added tests would have caught #661 and #665. The existing
it("completes when handler returns undefined")(added by #661, codifying the regressive behavior) is replaced; the new tests assert the actual Lambda runtime contract.Test plan
jest src/utils/handler.spec.ts— 20/20 passtslint --project tsconfig.json— cleanprettier --check src/utils/handler.{ts,spec.ts}— cleantsc --noEmit— cleanjest) — only 2 pre-existing failures insrc/index.spec.ts(nock-based, unrelated to handler logic; reproduce onmainwithout this PR)Related