Improvements in HTTPServerResponse.write() methods #84
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Test please :-) |
nethraravindran
approved these changes
Sep 3, 2018
This commit includes a fix for an embarrasing bug - we wrote to the response buffer only if it was nil, after allocating it. This meant, for N consecutive writes to it, only the first would be honored and N-1 would be ignored. This change also eagerly allocates the response buffer with an initial size of 2000, just like KituraNet does it. We don't keep the buffer as an optional anymore. We allocate it in the initializer because we are pretty sure that the HTTPServerResponse initializer always runs on an event loop.
pushkarnk
force-pushed
the
buffer-bug
branch
from
b72807f to
cc6a48e
Compare
|
Thanks for the review @ianpartridge ! I've added a test |
|
Found one more possibility in the code without the changes in this PR. Consider a response that writes twice to the ByteByffer. Because we submit the write task (creating a buffer and writing to it) on the event loop, the first write task may not have been invoked when the second one is about to be submitted. This results in a double allocation of the ByteBuffer, the first buffer being discarded. This pull request fixes that scenario too. P.S: A glance at the implementation of |
|
Tag a patch release? |
|
@ianpartridge tagged v1.0.4 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit includes a fix for an embarrasing bug - we wrote to the response buffer only if it was nil, after allocating it. This meant, for N consecutive writes to it, only the first would be honored and N-1 would be ignored. This change also eagerly allocates the response buffer with an initial size of 2000, just like KituraNet does it. We don't keep the buffer as an optional anymore. We allocate it in the initializer because we are pretty sure that the HTTPServerResponse initializer always runs on an event loop.