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

Fix #123 - request paused on exit #124

Merged
merged 5 commits into from
Nov 17, 2018
Merged

Fix #123 - request paused on exit #124

merged 5 commits into from
Nov 17, 2018

Conversation

mike-marcacci
Copy link
Collaborator

@mike-marcacci mike-marcacci commented Nov 16, 2018

It appears that subsequent operations which have the effect of pausing the request are already queued for this event frame. This fix adds setImmediate to push stream resumption to the next frame.

In the future, we should run tests with a small payload AND one that exceeds the maximum TCP packet size limit and node internal buffer. This would help identify this kind of bug.

It appears that subsequent operations which have the effect of pausing the
request are already queued for this event frame. This rux adds setImmediate to
push stream resumption to the next frame.

In the future, we should run tests with a small payload AND one that exceeds the
maximum TCP packet size limit and node internal buffer. This would help identify
this kind of bug.
Copy link
Owner

@jaydenseric jaydenseric left a comment

Choose a reason for hiding this comment

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

Thanks for the quick patch! It would have taken me a long time to work this out 😅

Let's add some comments to the source and tests explaining what the setImmediate and specific file size are for.

Oh, and a changelog.md entry!

@mike-marcacci
Copy link
Collaborator Author

Added! (Assuming this will be published in a patch-level release.)

@jaydenseric jaydenseric merged commit a287000 into master Nov 17, 2018
@jaydenseric jaydenseric deleted the fix/paused-request branch November 17, 2018 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants