Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

HTTPS vs HTTP failing to result in identical execution #892

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

HTTPS vs HTTP failing to result in identical execution #892

perezd opened this issue Apr 9, 2011 · 24 comments
Labels

Comments

@perezd
Copy link

perezd commented Apr 9, 2011

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
Copy link
Author

perezd commented Apr 10, 2011

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

@ghost ghost assigned ry Apr 10, 2011
@perezd
Copy link
Author

perezd commented Apr 10, 2011

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
Copy link

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

@perezd
Copy link
Author

perezd commented Apr 11, 2011

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
Copy link
Member

indutny commented Apr 13, 2011

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
Copy link
Author

perezd commented Apr 13, 2011

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

@indutny
Copy link
Member

indutny commented Apr 13, 2011

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

@perezd
Copy link
Author

perezd commented Apr 13, 2011

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

@indutny
Copy link
Member

indutny commented Apr 13, 2011

Is commenting those function helps in your case?

@ry
Copy link

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
Copy link
Member

indutny commented Apr 13, 2011

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

@indutny
Copy link
Member

indutny commented Apr 13, 2011

#917
@perezd please try that patch

@indutny
Copy link
Member

indutny commented Apr 13, 2011

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

@indutny
Copy link
Member

indutny commented Apr 13, 2011

@ry Drain variant:
#918

ry added a commit that referenced this issue Apr 13, 2011
@ry
Copy link

ry commented Apr 14, 2011

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

@indutny
Copy link
Member

indutny commented Apr 14, 2011

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

@ry ry closed this as completed in bb621f7 Apr 14, 2011
@perezd perezd reopened this Apr 14, 2011
@perezd
Copy link
Author

perezd commented Apr 14, 2011

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

@hanspinckaers
Copy link

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
Copy link

ry commented Apr 24, 2011

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

@hanspinckaers
Copy link

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
Copy link

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
Copy link

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
Copy link

pquerna commented Jul 7, 2011

@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
Copy link

koichik commented Jan 17, 2012

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

@koichik koichik closed this as completed Jan 17, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

8 participants