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

backpressure is broken #28

Closed
chemhack opened this issue May 19, 2013 · 13 comments
Closed

backpressure is broken #28

chemhack opened this issue May 19, 2013 · 13 comments

Comments

@chemhack
Copy link
Contributor

Recently I found that node eat a huge amount of memory in my production servers. After a brief test by downloading a 100M binary file I found this:

chart2 php

This is very clear that backpressure is broken. I have no idea why it happens as stream.pipe() should already handled that. It looks like the underlying node-spdy's problem. I.e. the 'drain' event of underlying socket was not propagate to the response stream.

Issue #24 may be related to this.

@indutny any ideas?

@igrigorik
Copy link
Owner

@chemhack which version of node? I'm guessing you're running the latest node-spdyproxy (from master)...?

@chemhack
Copy link
Contributor Author

node v0.8.23

yes, node-spdyproxy from master

@igrigorik
Copy link
Owner

Can you try 0.10+? We've had similar reports in the past which seems to have gone away after upgrade - see #21.

@chemhack
Copy link
Contributor Author

Same result

chart2-2 php

@chemhack
Copy link
Contributor Author

Update: This issue was observed in HTTP and HTTPS download.

@chemhack
Copy link
Contributor Author

Update again:

It has already been fixed 6 days ago by @indutny

spdy-http2/node-spdy@be33b5a

After back porting this commit to 1.5.0, the backpressure is working.

@igrigorik Why not update to newer version of node-spdy? Do you have a plan for this?

@indutny Can you release a 1.5.1 version of node-spdy on npm with commit spdy-http2/node-spdy@be33b5a patched as a temporary fix?

@igrigorik
Copy link
Owner

I'm happy to bump the version..

@indutny
Copy link
Contributor

indutny commented May 20, 2013

Oh, that's nice :) Didn't realize that this could be a problem for
spdyproxy ;)

Cheers,
Fedor.

On Mon, May 20, 2013 at 6:38 AM, Ilya Grigorik notifications@github.comwrote:

I'm happy to bump the version..


Reply to this email directly or view it on GitHubhttps://github.com//issues/28#issuecomment-18129823
.

@chemhack
Copy link
Contributor Author

@igrigorik How is going? I still didn't see version 1.5.1 of node-spdy. And node-spdyproxy has compatible issue with 1.8.x

@igrigorik
Copy link
Owner

@chemhack node-spdy v1.8.8 is up on NPM, so in theory as long as you install that, node-spdyproxy should work just fine.. I'm not sure where the 1.5.x requirement came from?

@chemhack
Copy link
Contributor Author

@igrigorik The problem is node-spdyproxy doesn't work with node-spdy 1.8.8. 1.5.0 just worked fine.

@igrigorik
Copy link
Owner

Oh, well then.. That's the actual problem we need to fix, instead of releasing 1.5.1.

@igrigorik
Copy link
Owner

Ok, testing locally, I believe spdy-http2/node-spdy#95 should fix the problem. /cc @indutny

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

No branches or pull requests

3 participants