Override Response.setTimeout to make response timeout handler work #69
Conversation
lib/response.js
Outdated
| @@ -46,7 +46,18 @@ function Response (req, onEnd, reject) { | |||
|
|
|||
| util.inherits(Response, http.ServerResponse) | |||
|
|
|||
| let timeoutHandle | |||
There was a problem hiding this comment.
this reference should be stored in the constructor function Response
There was a problem hiding this comment.
@Eomm Id don't really get it. do you mean I should set it as a property and access it as this.timeoutHandle?
lib/response.js
Outdated
| @@ -46,7 +46,18 @@ function Response (req, onEnd, reject) { | |||
|
|
|||
| util.inherits(Response, http.ServerResponse) | |||
|
|
|||
| let timeoutHandle | |||
| Response.prototype.setTimeout = function (msecs, callback) { | |||
| if (callback) { | |||
There was a problem hiding this comment.
We should not permit to call this function without a callback, otherwise, the user can't understand why it is not working
There was a problem hiding this comment.
ok, shall I throw an error if no callback is specified?
There was a problem hiding this comment.
According to the http docs, the callback is optional: https://nodejs.org/api/http.html#http_response_settimeout_msecs_callback
lib/response.js
Outdated
| Response.prototype.writeHead = function () { | ||
| if (timeoutHandle) { | ||
| clearTimeout(timeoutHandle) |
There was a problem hiding this comment.
Could be the write function a better place where clear this timeout?
There was a problem hiding this comment.
@Eomm ok, I'll move it there
lib/response.js
Outdated
| let timeoutHandle | ||
| Response.prototype.setTimeout = function (msecs, callback) { | ||
| if (callback) { | ||
| timeoutHandle = setTimeout(callback, msecs) |
There was a problem hiding this comment.
We will need to emit the 'timeout' event when the setTimeout triggers (independently of the presence of the callback)
See https://nodejs.org/api/net.html#net_socket_settimeout_timeout_callback for more details.
There was a problem hiding this comment.
@mcollina according to the docs, the timeout event should be emitted by the socket. how can I cet a reference to the underlying socket?
There was a problem hiding this comment.
is calling this.socket.emit('timeout') enough?
|
Somebody please, merge this PR. |
Checklist
npm run testandnpm run benchmark