Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add response lifecycle tracking and checks #4257

Merged
merged 2 commits into from
Jun 10, 2021

Conversation

kanongil
Copy link
Contributor

@kanongil kanongil commented Jun 4, 2021

This is in response to #4256.

I added asserts to verify the expected state so this kind of error are less likely to be introduced in future code. This also caught another error, where the marshal() method could sometimes be called after close().

lib/response.js Outdated Show resolved Hide resolved
lib/response.js Outdated Show resolved Hide resolved
lib/response.js Outdated Show resolved Hide resolved
test/response.js Show resolved Hide resolved
@nlf
Copy link
Member

nlf commented Jun 4, 2021

as a note, the failing builds here are unrelated to these changes and are fixed in #4258

@kanongil
Copy link
Contributor Author

kanongil commented Jun 6, 2021

One outstanding question is whether the asserts should remain in the code. While the only purpose they serve is to catch hapi internal errors, I think that it is warranted to ensure we maintain consistency and limit the risk of regressing.

Copy link
Member

@nlf nlf left a comment

Choose a reason for hiding this comment

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

this looks good to me 👍

@nlf
Copy link
Member

nlf commented Jun 7, 2021

One outstanding question is whether the asserts should remain in the code. While the only purpose they serve is to catch hapi internal errors, I think that it is warranted to ensure we maintain consistency and limit the risk of regressing.

i agree, i think keeping the assertions in place for now is fine. i'd rather we blow up with an error a user can open an issue with than do something out of order and send incorrect responses for ✨ reasons ✨

@kanongil
Copy link
Contributor Author

kanongil commented Jun 8, 2021

Rebased to fix the test failures.

@devinivy devinivy added the bug Bug or defect label Jun 10, 2021
@devinivy devinivy added this to the 20.1.4 milestone Jun 10, 2021
Copy link
Member

@devinivy devinivy left a comment

Choose a reason for hiding this comment

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

I think it is slightly bold for us to introduce these assertions, but I also appreciate that it's probably best for the longterm. This looks great, thanks for the contribution as always, Gil. Release on the way.

@devinivy devinivy merged commit 74d6b52 into hapijs:master Jun 10, 2021
@kanongil kanongil deleted the response-state branch October 4, 2021 08:29
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.

3 participants