search

spdy-http2 / node-spdy

SPDY server on Node.js
2.81k stars 196 forks source link

create backpressure on socket write buffering #175

Closed ploer closed 10 years ago

ploer commented 10 years ago

See issue #174 for details of the issue this addresses.

This change addresses the issue in my test environment, and node-spdyproxy seems stable with the change under both 0.10.x and 0.8.x.

Tests are failing under 0.8.x but it looks like that is not due to my change (latest travis run on master branch also failed 0.8.x on the same cases).

The change in _writeData is somewhat duplicative with the window exhaustion check immediately below, but it seemed clearer to me to express the two checks separately. Happy to combine them or factor into a function if you prefer.

On socket 'drain' event, each stream's _drainSink() is called unconditionally. Presumably this could happen while there are buffered writes in the sink due to the window being exhausted, and so it would be inappropriate to do any new writes, but the drainSink mechanism should deal with this fine by just re-buffering them if they are not ready. Simplicity seemed preferable to trying to prematurely optimize that away, but if you see a performance concern (possibly with a writer who is buffering very large amounts into the spdy code?), that could be easily changed, maybe just with an early exit out of _drainSink() if either the window is still full or the socket is still buffering.

Let me know what you think.

Thanks!

indutny commented 10 years ago

Only one minor nit, otherwise looking good. Does it fix the problem for you?

ploer commented 10 years ago

Reverted the extraneous travis.yml change.

Yes, these changes fix the problem for me.

indutny commented 10 years ago

What could you say about https://travis-ci.org/indutny/node-spdy/jobs/36579100 ? node-spdy does support node.js v0.8, and I can't really afford to break it.

ploer commented 10 years ago

Well, that was my comment about 0.8.x tests appearing to already be broken: https://travis-ci.org/indutny/node-spdy/builds/31463661 (which should be the link to the 1.28.1 test results)

It looks to me like this commit is failing for the same reason as that one, right? So I didn't think this change was causing it.

indutny commented 10 years ago

Oh, sorry! I'll look into this patch ASAP, but right now I'm onto a different thing, sorry!

indutny commented 10 years ago

Thank you, landed and released! :)

ploer commented 10 years ago

Much appreciated!