-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
as a note, the failing builds here are unrelated to these changes and are fixed in #4258 |
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. |
There was a problem hiding this 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 👍
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 ✨ |
Rebased to fix the test failures. |
There was a problem hiding this 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.
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 afterclose()
.