-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
Requests after long delays halt on tls.js:182 #2304
Comments
Very interesting... I think I figured it out, will let you know once I'll test the change :) Not sure if I will be able to provide a proper test case, though. |
If you can figure out some sort of workaround without having to wait for an iojs update, that'd be really amazing too. I need this working fairly badly. Maybe like a try { request } catch { //new-session-thing }? Dunno really. |
The only workaround that I could think of at the moment is following: var https = require('https');
var agent = new https.Agent();
agent._getSession = function nop() {};
var request = require('request').defaults({ agent: agent }); This is the only way to disable session cache atm. |
I just tried that, at least I believe I set it up right.
to be exact. But I still got the same error. |
Here is some debugging data, just for reference:
|
@sadtaco let me test it locally, perhaps I did a mistake there... |
@sadtaco appears to be fixing problem for me. Are you sure that you are reusing the |
I don't get what you mean. Are you asking if I'm resetting the request variable to something else elsewhere? |
@sadtaco I wasn't sure if you was doing requests from a single js file, or not. It is very odd that the problem is still happening for you, as it does not seem to be reproducible on my machine with these changes (and it is without them). I'm going to work on the test case, and will submit a PR as soon as I'll be able to reproduce it reliably. |
Yeah, in a single file. Not really different from the test case I posted. Only difference is that it's triggered by an app.get('/...') instead of the setInterval. |
Argh, I think I know why it wasn't working for you. May I ask you to give a try to: var https = require('https');
var util = require('util');
function Agent() {
https.Agent.apply(this, arguments);
}
util.inherits(Agent, https.Agent);
Agent.prototype._getSession = function nop() {};
var request = require('request').defaults({ agentClass: Agent }); |
Ok, I know how to reproduce it ;) Hehe, will post a PR soon! |
Whoa, test took more lines than the fix #2312 ;) |
Nope, that work around didn't work still. Hope the fix does, though. Is there a way to checkout an iojs with the fix or something? |
`enableTicketKeyCallback` and `onticketkeycallback` could be potentially used to renew the TLS Session Tickets before they expire. However this commit will introduce it only for private use yet, because we are not sure about the API, and already need this feature for testing. See: nodejs#2304 PR-URL: nodejs#2312 Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
When TLS Session Ticket is renewed by server - no Certificate record is to the client. We are prepared for empty certificate in this case, but this relies on the session reuse check, which was implemented incorrectly and was returning false when the TLS Session Ticket was renewed. Use session reuse check provided by OpenSSL instead. Fix: nodejs#2304 PR-URL: nodejs#2312 Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
`enableTicketKeyCallback` and `onticketkeycallback` could be potentially used to renew the TLS Session Tickets before they expire. However this commit will introduce it only for private use yet, because we are not sure about the API, and already need this feature for testing. See: #2304 PR-URL: #2312 Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
When TLS Session Ticket is renewed by server - no Certificate record is to the client. We are prepared for empty certificate in this case, but this relies on the session reuse check, which was implemented incorrectly and was returning false when the TLS Session Ticket was renewed. Use session reuse check provided by OpenSSL instead. Fix: #2304 PR-URL: #2312 Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Closed as landed of #2312 . |
`enableTicketKeyCallback` and `onticketkeycallback` could be potentially used to renew the TLS Session Tickets before they expire. However this commit will introduce it only for private use yet, because we are not sure about the API, and already need this feature for testing. See: nodejs#2304 PR-URL: nodejs#2312 Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
When TLS Session Ticket is renewed by server - no Certificate record is to the client. We are prepared for empty certificate in this case, but this relies on the session reuse check, which was implemented incorrectly and was returning false when the TLS Session Ticket was renewed. Use session reuse check provided by OpenSSL instead. Fix: nodejs#2304 PR-URL: nodejs#2312 Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
`enableTicketKeyCallback` and `onticketkeycallback` could be potentially used to renew the TLS Session Tickets before they expire. However this commit will introduce it only for private use yet, because we are not sure about the API, and already need this feature for testing. See: #2304 PR-URL: #2312 Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
When TLS Session Ticket is renewed by server - no Certificate record is to the client. We are prepared for empty certificate in this case, but this relies on the session reuse check, which was implemented incorrectly and was returning false when the TLS Session Ticket was renewed. Use session reuse check provided by OpenSSL instead. Fix: #2304 PR-URL: #2312 Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
http://pastebin.com/LLX6isjZ
(Not sure if the express part matters, but takes 30+ minutes to do a test to see if it fails as expected so this the least I got it cut down to to confirm the bug)
Just run this, and after about 30-60 minutes you should get
@indutny
The text was updated successfully, but these errors were encountered: