Skip to content

fix(response): make send() a no-op when response already finished (#134)#135

Merged
VikramAditya33 merged 1 commit into
FOSSFORGE:mainfrom
HirenGajjar:fix/uws-response-send-no-op-when-finished
May 14, 2026
Merged

fix(response): make send() a no-op when response already finished (#134)#135
VikramAditya33 merged 1 commit into
FOSSFORGE:mainfrom
HirenGajjar:fix/uws-response-send-no-op-when-finished

Conversation

@HirenGajjar
Copy link
Copy Markdown
Contributor

@HirenGajjar HirenGajjar commented May 13, 2026

Fixes #134.

When a handler sends a response then throws, NestJS's exception filter calls adapter.end(response) to gracefully close the response. This delegated to UwsResponse.send(), which threw Error: Response already sent because this.finished was already true. The throw was logged as a spurious "Unhandled route error" even though the client had already received their response.

Change

In src/http/core/response.ts, changed the guard from a throw to a silent return — matching Express and Fastify behavior:

if (this.finished) {
  return;
}

Testing

Added test/http/response-send-after-finished.e2e.spec.ts with two tests:

  • Response is still delivered correctly to the client
  • "Response already sent" is no longer logged when a handler throws after send()

Both pass. Pre-existing failures in static-file-serving.e2e.spec.ts were observed on clean main and are unrelated to this change.

Summary by CodeRabbit

Bug Fixes

  • Response handler now silently ignores duplicate send() calls instead of throwing an error, allowing graceful handling of redundant requests.

Review Change Stack

@HirenGajjar HirenGajjar changed the title fix(response): make send() a no-op when response already finished (#134)m fix(response): make send() a no-op when response already finished (#134) May 13, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 13, 2026

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 1d065822-6ac1-48fe-ab0c-7dae5482a376

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • ✅ Review completed - (🔄 Check again to review again)
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@VikramAditya33
Copy link
Copy Markdown
Collaborator

The fix is straightforward but I think we don't need explicit e2e particularly to test this. E2E tests are not something we simply test for a branch or a smaller change but it decides a whole feature. If you have to add tests then add unit tests to test this branch/functionality. Hope it makes sense.

Fixes FOSSFORGE#134

UwsResponse.send() threw 'Response already sent' when called on an
already-finished response. This occurred during NestJS's normal
exception-filter flow: when a handler sends a response then throws,
NestJS calls adapter.end(response) for graceful cleanup, which
delegated to send() and triggered the throw.

The throw bubbled into RouteRegistry.handleException() and was logged
as a spurious 'Unhandled route error' stack trace, even though the
client had already received their response successfully.

Changed the guard from a throw to a silent return, matching the
behavior of Express and Fastify.

Updated existing unit test in response.spec.ts to assert the new
no-op behavior (verifies end() is called exactly once, not twice).
@HirenGajjar HirenGajjar force-pushed the fix/uws-response-send-no-op-when-finished branch from 174d6f6 to 2b92d33 Compare May 13, 2026 17:50
@HirenGajjar
Copy link
Copy Markdown
Contributor Author

Updated — replaced the e2e test with a unit test in src/http/core/response.spec.ts. There was already a test asserting the old throwing behavior; I flipped it to assert the no-op (also verifies end() is called exactly once). All 133 unit tests pass.

@VikramAditya33 VikramAditya33 self-requested a review May 13, 2026 17:54
Copy link
Copy Markdown
Collaborator

@VikramAditya33 VikramAditya33 left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you

@VikramAditya33 VikramAditya33 merged commit 502f48d into FOSSFORGE:main May 14, 2026
4 checks passed
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.

UwsResponse.send() throws when response is already finished

2 participants