Skip to content

Support handlers calling res.destroy() to abort sending a response#150

Merged
Marsup merged 2 commits intohapijs:masterfrom
kanongil:response-abort
Aug 13, 2025
Merged

Support handlers calling res.destroy() to abort sending a response#150
Marsup merged 2 commits intohapijs:masterfrom
kanongil:response-abort

Conversation

@kanongil
Copy link
Contributor

When calling res.destroy([err]) before 'finish', shot will currently either do nothing and never return, or reject/throw an unhandled exception.

This does not seem desirable. This is fixed in this PR by detecting aborted responses, and exposing such through a new aborted property. Depending on when the abort happens, more or less of the response can be filled in along with it.

I don't think this is a breaking change, since it only changes the behaviour of previously failing inject() calls. It can however affect code that relies on the buggy behaviour. One example is this test from hapi, where inject will now return a value. But the test still passes. In fact all hapi tests still pass.

Note that I'm not really a fan of the new aborted property, since you will need to remember to check for it, as the response might otherwise look complete. Ideally this property would only be exposed with an option, and normally just reject with an error. This is however more of a breaking change, and conflicts with how hapi uses this module.

@kanongil kanongil added the bug Bug or defect label Aug 12, 2025
lib/response.js Outdated
});
};

const abort = finalize.bind(this, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Bind won't work on arrow functions, shouldn't we just inline this below with a () => finalize(true)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bind works for the purpose (adding true) as the first argument.

Granted the this is not used. () => finalize(true) is fine as well. Though, it cannot be inlined, as it needs to be removed again.

@Marsup Marsup added this to the 6.0.2 milestone Aug 13, 2025
@Marsup Marsup self-assigned this Aug 13, 2025
@Marsup
Copy link
Contributor

Marsup commented Aug 13, 2025

Oh, looks like your other PR conflicted with this one.

@Marsup Marsup merged commit 2523994 into hapijs:master Aug 13, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug or defect

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants