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

Large file uploads getting data lost and never finishing over HTTPS in 0.10.0 #5023

Closed
geoffreak opened this issue Mar 14, 2013 · 11 comments
Closed
Assignees
Milestone

Comments

@geoffreak
Copy link

All code referenced in this issue can be found in this gist.

I've found an issue with HTTPS servers where large file uploads have data lost (if not used right away). I tested a 8MB text file that contained This is a testfile! repeated excessively.

In the example code, the only difference between httpsServerFail and httpServerSuccess is that they use the https vs http modules, yet only the non-HTTPS version will receive all the data from a file. My 8MB test file would only end up creating a file on the server that was around 140-160KB in addition to never causing an end event. Files that are only a few KB in size have no issues. Remember that no such issues exist with HTTP servers.

In the example code, a successful upload should have the server print the following:

data
timeout
end

Whereas an unsuccessful upload will only print the following (and hang the client)

data
timeout

To simplify things in the clients, I used the request module, but I verified that it wasn't the cause by also running the following curl requests via the terminal that produced identical results.

curl https://localhost:8091 -X PUT -T testfile.txt  -k
curl http://localhost:8091 -X PUT -T testfile.txt 
@isaacs
Copy link

isaacs commented Mar 14, 2013

@indutny would you like to look into this? Sounds like the CryptoStream is locking up at some point?

@ghost ghost assigned indutny Mar 14, 2013
@indutny
Copy link
Member

indutny commented Mar 14, 2013

Sure.

@indutny
Copy link
Member

indutny commented Mar 14, 2013

For tomorrow me - this might be related to this https://github.com/joyent/node/blob/master/lib/tls.js#L316 , considering comment about 160 kb of incoming data. Though, I haven't looked really deep yet.

indutny added a commit to indutny/node that referenced this issue Mar 15, 2013
Fix stucked CryptoStream behaviour, happening when one of the sides
locks-up in queued state.

fix nodejs#5023
@indutny
Copy link
Member

indutny commented Mar 15, 2013

See #5034

@indutny
Copy link
Member

indutny commented Mar 15, 2013

Can you please verify that your test works with my fix?

@geoffreak
Copy link
Author

Sure, I'm trying to figure out how run your modified version of Node now.

@indutny
Copy link
Member

indutny commented Mar 15, 2013

Just checkout this branch, run "./configure && make -j4 && ./node server.js" and test it with curl or whatever.

@geoffreak
Copy link
Author

Okay, it appears to be working. I see the end on the server, the client finishes, and the uploaded file is the full size. I tested against this branch.

@indutny
Copy link
Member

indutny commented Mar 17, 2013

Fixed in 14a8fb8.

@indutny
Copy link
Member

indutny commented Mar 17, 2013

Ouch... that should be landed in 0.10 too.

indutny added a commit that referenced this issue Mar 17, 2013
Fix stucked CryptoStream behaviour, happening when one of the sides
locks-up in queued state.

fix #5023
@indutny
Copy link
Member

indutny commented Mar 17, 2013

And b5ddc0c in 0.10

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants