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

Add highwatermark to readable in buffer.tostream #551

Closed
wants to merge 1 commit into from
Closed

Add highwatermark to readable in buffer.tostream #551

wants to merge 1 commit into from

Conversation

rclark
Copy link
Contributor

@rclark rclark commented Mar 29, 2015

I ran into this issue with recursive nextTick calls again and thought I'd just throw in a PR that helps: see #158 (comment). Again, while this problem does represent a problem in the node.js stream api, adding this is a very simple workaround.

p.s. this time I encountered the error when uploading a ~25MB zip file to lambda.

@lsegal
Copy link
Contributor

lsegal commented May 8, 2015

I'm not sure we can pull this in. This change causes some of our progress event tests to fail because adding a HWM causes Node.js to stop emitting granular progress events, and based on the documentation, it looks like this change would cause Node.js to buffer 5mb chunks into memory, which would potentially incur a significant memory performance regression into the SDK.

Out of curiosity, are you running into this on a small EC2 instance? We've noticed this behavior on t2.micro type instances because of the known issue in 0.10.x that you mentioned in #158. I think the better solution here might be to upgrade to 0.12.x now that it is an officially supported version, which should resolve your nextTick issue.

Let me know what you think.

@rclark
Copy link
Contributor Author

rclark commented May 8, 2015

I think the better solution here might be to upgrade to 0.12.x

This sounds fine to me. We encounter this problem rarely and can usually find a way around it. I've seen it crop up on all kinds of EC2s. I've never run a t2 instance so I'm afraid I can't speak to that.

@lsegal
Copy link
Contributor

lsegal commented May 8, 2015

@rclark in that case I'm going to mark this PR as closed, but thanks for the extra info!

@lock
Copy link

lock bot commented Sep 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants