HTTPS vs HTTP failing to result in identical execution #892

Closed
perezd opened this Issue Apr 9, 2011 · 24 comments

8 participants

@perezd

I've been trying hard to trace this error out, and I believe it to be an issue inside of node's HTTPS libraries. I've created a repeatable snip you can run to produce the error.

https://gist.github.com/911661

Inside the gist is an explanation of the conditions to test, and the scenarios I have experienced. I am on Mac OS X 10.6.7, I've run this test on 0.4.5 and 0.5.0-pre.

Any advice is appreciated.

@perezd

Here is a diff and verbose output of this exact operation, using curl. Cloudant's response looks identical
https://gist.github.com/912568

@ry ry was assigned Apr 10, 2011
@perezd

I've updated the gist based on our Saturday conversation. There are two HTTP echo servers that just return 201, runs cradle just fine.

on the echo HTTP servers, both SSL and not SSL work. You'll have to self sign something for the HTTPS local test.
You can simply go through commenting/uncommenting the various environments and see the issue very clearly.

Let me know if I can help with anything else!

@steadicat

I rewrote the test case without Cradle, at it seems to work fine (unless I'm missing something):
http://friendpaste.com/1xIfYIrlsgyuodJheNy24j

@perezd

Interestingly, the test case I've provided works an a seemingly identical macbook pro here in my office running the 0.4.5 release of node. this is really strange :/ @steadicat, your script works fine for me too, but that makes me wonder even further what @cloudhead could be doing wrong, if anything...

@indutny
Node.js Foundation member

Looks like this is related to problem I'm experiencing: sometimes tls server stops receiving packets b/c of CryptoStream.prototype.pause function. If you'll delete pause and resume from CryptoStream - it should work fine!

@perezd

hmmmm, that seems like a really bad idea to remove those.

@indutny
Node.js Foundation member

I'm not proposing that as solution. Just trying to find root of all evil in tls :)

@perezd

yeah, definitely something wrong there, we've been diagnosing it. Its a very subtle issue, whatever it is.

@indutny
Node.js Foundation member

Is commenting those function helps in your case?

@ry
ry commented Apr 13, 2011

@donnerjack13589, i believe he doesn't terminate with a "pause" - he terminates after a "resume" - but @perezd, you should try it

@indutny
Node.js Foundation member

@ry, I'd found a problem, creating pull request, wait a minute

@indutny
Node.js Foundation member

#917
@perezd please try that patch

@indutny
Node.js Foundation member

Hope if that works you'll have time to review my NPN pull request ;)

@indutny
Node.js Foundation member

@ry Drain variant:
#918

@ry ry added a commit that referenced this issue Apr 13, 2011
@ry ry Test to demonstrate #892 296ff04
@ry
ry commented Apr 14, 2011

fix https://gist.github.com/918661. waiting for @perezd to verify fix.

@indutny
Node.js Foundation member

Interesting... but how stream.pipe will know that is' ready again if it'll return false? we need to emit 'drain' anyway

@ry ry added a commit that closed this issue Apr 14, 2011
@ry ry CryptoStream.write returns false when queue > 128kb
Previously the return value of write was dependent on if it was paused or
not which was causing a strange error demoed in the previous commit.

Fixes #892
bb621f7
@ry ry closed this in bb621f7 Apr 14, 2011
@perezd perezd reopened this Apr 14, 2011
@perezd

This patch doesn't work as well, unfortunately, check out the following exceptions I am getting + debug trace: https://gist.github.com/0b4fdf9376825d4de151

@TooTallNate TooTallNate pushed a commit that referenced this issue Apr 18, 2011
@ry ry CryptoStream.write returns false when queue > 128kb
Previously the return value of write was dependent on if it was paused or
not which was causing a strange error demoed in the previous commit.

Fixes #892
bb621f7
@HansPinckaers

I also still have this issue. I really needed this to work on the short term, so I commented the last sentence of the resume function: "this.pair._cycle();" out. Now it works every time. It's probably a bad thing to do, I know. Maybe it helps fixing this issue.

@ry
ry commented Apr 24, 2011

@HansPinckaers can you tell us more - what exactly is error - can you reproduce?

@HansPinckaers

Well this is weird. I used to have the same error as perezd in the starting post. But i can't reproduce it anymore. If it happens again, i let you know.

@csosborn

I think I've run into the same problem, or else something very similar. When I attempt to pipe a readable file stream into a writable TLS stream (an http ClientRequest) the TLS stream write() eventually returns false, causing pipe() to pause the file stream. It never emits a drain event, so the file stream never resumes and the whole thing hangs. I have a simple demonstration here: https://gist.github.com/1045054. If you change it to use http instead of https it will run to completion and print "SUCCESS", but with https it will hang.

I'm running v0.4.8 on OS X.

@csosborn csosborn pushed a commit that referenced this issue Jun 24, 2011
Chris Osborn Fix bug #892.
Piping a stream to an HTTPS ClientRequest would stall if the request's
write() method returned false; the readable stream would pause and
never resume because the the request would never emit a drain event.
It turns out that the request stream did not listen to the underlying
TLS socket drain event because it was expecting the socket to invoke
its "ondrain" method instead. Now it does.
692e807
@csosborn

I think I tracked this down. When an HTTP client request is associated with a socket (either plain net or tls) it does not listen to the socket's 'drain' event and re-emit as I'd expected. Instead, it relies on the socket to call its own ondrain() method as an optimization. See https://github.com/Sitelier/node/blob/tls_bug_fix/lib/http.js#L941. The problem was that normal net sockets know about this optimization and play along, but TLS sockets do not. I've brought TLS sockets up to speed and the test i posted earlier now passes.

This looks like a pretty clear-cut bug so I went ahead and put in a pull request.

@pquerna

@csosborn: Yep, this looks real. Could you rework your gist above into a test case as part of the pull request? Then we'd prevent this from regression in the future.

@koichik

Probably, this has fixed in v0.5.8 (#1775), closing.
Please reopen if this is still an issue.

@koichik koichik closed this Jan 17, 2012
@isaacs isaacs added a commit to isaacs/node that referenced this issue Mar 6, 2013
@isaacs isaacs test: Pass cli flags in pummel/test-regress-GH-892 892a20d
@isaacs isaacs added a commit to isaacs/node that referenced this issue Mar 6, 2013
@isaacs isaacs test: Pass cli flags in pummel/test-regress-GH-892 78e1172
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment