-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 closed state to HTTP::Server::Response #6477
Add closed state to HTTP::Server::Response #6477
Conversation
998a533
to
82dc655
Compare
def closed? | ||
@closed | ||
end | ||
|
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 guess this should be checked in unbuffered_write
?
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.
IO::Buffered#write
already calls check_open
. This is ensured with the expect_raises
expectation.
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.
Missing closed = false in reset
@@ -154,6 +159,7 @@ class HTTP::Server | |||
@sync = false | |||
@flush_on_newline = false | |||
@chunked = false | |||
@closed = false |
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.
@asterite This is in #reset
. I'm not sure how useful this is, because when a request is closed, at least the headers has already been written to the underlying IO. This effectively can't be reset because the response is already sent.
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.
Oh, cool. Reset is useful, can't remember now why, but response is reused for keep alive, check request processor.
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.
Oh yeah, RequestProcessor
resets the response to avoid allocating a new instance for each iteration.
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'll add a comment about this.
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.
Thank you @straight-shoota 👍
Fixes #6465