Fix to timeout not being respected#27
Conversation
|
|
||
| if (timeout) { | ||
| req.setTimeout(timeout, function() { | ||
| req.on('timeout', ()=> { |
There was a problem hiding this comment.
Minor style issue: space between the arrow and the parens.
|
|
||
| var options = getOptions(url, port, protocol); | ||
|
|
||
| if (timeout) |
There was a problem hiding this comment.
Many JS style guides prohibit if without a block, always requiring braces even for a single line. How do you feel about that?
There was a problem hiding this comment.
I feel they are right, specially if this standard was used throughout this file. I'll be fixing this and the other style issue.
|
Looks good to me. |
| var options = getOptions(url, port, protocol); | ||
|
|
||
| if (timeout) { | ||
| options['timeout'] = timeout; |
There was a problem hiding this comment.
I would do that with Object.assign.
There was a problem hiding this comment.
Also why not just extend getOptions?
| it('should timeout at expected time', function(done) { | ||
| const startTime = new Date(); | ||
| getSSLCertificate | ||
| .get('59.96.120.246', 1400) // Not sure which unreachable IP to put in here |
There was a problem hiding this comment.
Probably any IP controlled by IANA or internal one defined by IANA.
Please create a separate branch instead of using the |
|
@johnnysprinkles could you please merge the latest changes to master |
|
I'm not an owner of this library... |
5 participants
When setting an unreachable IP from your network as the host address, the script fails to return a timeout error at the expected time. I tried to minimally change your script to fix this by using the ClientRequest 'timeout' option when calling 'https.get()'. Also I'm using 'ClientRequest.on('timeout)...' to listen the timeout event. The added test can be used to verify that the error occurs currently. This solution can be improved however, specially the IP used for the test, as someone could have a server running there.