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.

Sign up for GitHub

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

Jump to bottom

fix: pause while piping #76

Merged
merged 1 commit into from
May 7, 2020
Merged

fix: pause while piping #76

merged 1 commit into from
May 7, 2020

Conversation

ronag
Copy link
Member

@ronag ronag commented May 6, 2020

Fixes: #52

@ronag
Copy link
Member Author

ronag commented May 6, 2020

Missing tests

@ronag ronag force-pushed the pause-piping branch 4 times, most recently from 4715e7b to d948a3f Compare May 6, 2020 23:42
ronag
ronag commented May 6, 2020
View reviewed changes
lib/client.js Outdated Show resolved Hide resolved
@ronag ronag marked this pull request as ready for review May 6, 2020 23:43
@ronag ronag force-pushed the pause-piping branch 3 times, most recently from 176cfeb to fcc9188 Compare May 7, 2020 00:04
@ronag ronag mentioned this pull request May 7, 2020
Closed
@ronag
Copy link
Member Author

ronag commented May 7, 2020

This should not land until the strange behavior is either understood or a better implementation is found.

mcollina
mcollina approved these changes May 7, 2020
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@ronag
Copy link
Member Author

ronag commented May 7, 2020

Don't merge yet. Need to fix so that client closed connection and doesn't assume server does it.

@ronag ronag self-assigned this May 7, 2020
@ronag ronag force-pushed the pause-piping branch 2 times, most recently from 4f1e4df to d171780 Compare May 7, 2020 14:53
@ronag ronag requested a review from mcollina May 7, 2020 14:54
@ronag
Copy link
Member Author

ronag commented May 7, 2020 •
edited

@mcollina: PTAL. I've made this very strict right now just to be sure. It will reset the connection for any request that contains a body. Please let me know if you think this is too strict.

@ronag ronag force-pushed the pause-piping branch 3 times, most recently from 38ac327 to 27c5e94 Compare May 7, 2020 15:02
mcollina
mcollina approved these changes May 7, 2020
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@ronag ronag merged commit b8c9d59 into nodejs:master May 7, 2020
@ronag ronag deleted the pause-piping branch May 7, 2020 21:06
@mcollina mcollina mentioned this pull request Jan 17, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

Assignees

@ronag ronag

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

pause queue while piping
2 participants
@ronag @mcollina