Fix request timeout no working

[aleung]

This PR fixed #754 and #739

There are two changes:

1. Set socket.connecting to true to allow request module creates timeout timer.
2. Emit ClientRequest.connect event after connection delay, just before sending response. Before modification, it emits too early and make request module clears the connection timeout timer.

[coveralls]

Coverage increased (+0.0007%) to 96.538% when pulling 5701fa6 on aleung:fix-754 into 9ee2984 on node-nock:master.

[aleung]

Updated a failed test case which covers Restify use case #79.

In that test case the ClientRequest ends until connect event is received. But actually Restify ends the req on socket event. The test case was incorrect.

See #79 (comment)

[ianwsperber]

Thanks for working on this @aleung. Could you add a test case asserting the new behavior?

[coveralls]

Coverage increased (+0.0007%) to 96.538% when pulling 39afb7e on aleung:fix-754 into f6d3e76 on node-nock:master.

[aleung]

@ianwsperber The test case was there (test_timeout.js). It started failing with request v2.76.0 (see this comment). The test suite used request v2.71.0 which won't fail even before my fix. I have pushed a commit to update it to v2.79.0.

[ianwsperber]

@aleung Because I'm new to the project I would like to wait on feedback from @vrinek or others before continuing. If we don't get any additional feedback within the week we can move this PR forward ourselves. Thanks!

[coveralls]

Coverage increased (+0.005%) to 92.666% when pulling 343dc5d on aleung:fix-754 into 5856cde on node-nock:master.

[aleung]

@ianwsperber Can we go forward? I just rebase my fix to master latest.

[ianwsperber]

Would feel more confident if we already had #832. I will merge in the next few days @aleung, once we're confident 9.0.7 is settled

[silentroach]

~/.gitignore is a place for that

[ianwsperber]

@aleung Are you sure we don't need to emit a connect event here anymore? Might this cause a regression of #79?

[ianwsperber]

Thanks for your help and patience on this one @aleung. Your changes will go out in v9.0.8 today.

[matteocontrini]

I think this broke something.
I have a test that uses socketDelay to test request timeouts and it started failing after updating to 9.0.8.
I haven't tried to isolate the issue yet, but it should look like this:

nock('http://myservice.it')
	.get('/')
	.query(true)
	.socketDelay(2000)
	.reply(400);

request({ url: 'http://myservice.it/?something', timeout: 1000 }, (err) => {
	// err is falsy
});

[lpinca]

I hit the same issue. socketDelay() no longer works.

[ianwsperber]

Thanks @matteocontrini & @lpinca, I've created #839 to track the bug.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue and add a reference to this one if it’s related. Thank you!