chore: opinionated clean up of once callbacks #180
Conversation
lib/HttpClient.js
Outdated
| @@ -16,7 +16,7 @@ var util = require('util'); | |||
| var assert = require('assert-plus'); | |||
| var backoff = require('backoff'); | |||
| var mime = require('mime'); | |||
| var once = require('once'); | |||
| var once = require('once').strict; | |||
There was a problem hiding this comment.
nit: I think I'd find it more obvious what behavior is used by each "onced" callback if each was explicitly created with once.strict below instead of once. Reviewing this PR I got confused: I did not understand what had changed since the call sites were using once: I thought they were still using the non-strict versions.
This is highly subjective though, I'm fine if you prefer this style!
lib/HttpClient.js
Outdated
| @@ -407,6 +385,9 @@ function rawRequest(opts, cb) { | |||
| socket.setKeepAlive(true); | |||
| } | |||
|
|
|||
| // eagerly return here with the request. it's still very possible | |||
| // for the request to timeout at this point. if that happens, a | |||
| // RequestTimeoutError will be emitted as part of the result event. | |||
| cb(null, req); | |||
There was a problem hiding this comment.
Is there a specific reason why we call cb here instead of in the proto.request callback? Without being familiar with restify client's implementation, I would think the main difference would be that by calling cb here it's called when the connection to the peer is established, but possibly before the first part of the HTTP response was received, whereas by calling it from proto.request's callback, it'd be called only after the first part of the HTTP response was received.
Is that distinction useful? If not, would that allow us to refactor the code in a way that doesn't require us to still write some error prone code to avoid calling cb more than once?
There was a problem hiding this comment.
You're spot on, I believe the HttpClient works by having "two" areas for error propagation:
httpclient.get('/foo', function(err, req) {
// err can be DNSTimeout or ConnectTimeout, in which case the 'result' event is never fired
req.on('result', function(err, res) {
// err can be RequestTimeout or http level err
});
});I'm not sure if the distinction is useful TBH, and it requires consumers to have to check for errors in two places.
| emitAfter(req, null, err); | ||
| }); | ||
| req._forcedAbortErr = err; | ||
| req.abort(); |
There was a problem hiding this comment.
nit/thinking out loud: do we want to assert on req here since it seems we rely on it being a request object? I'm thinking about it more from a code documentation perspective.
| cb(err, req); | ||
| emitResult(err, req, null); | ||
| emitAfter(req, null, err); | ||
| req._forcedAbortErr = err; |
There was a problem hiding this comment.
General question: what about using a symbol here to avoid stomping on consumer's properties?
There was a problem hiding this comment.
@hekike and I were chatting about this the other day - we'd like to add an "engines" field into package.json so we can begin to safely use ES6 stuff, but that'd be reserved for a breaking change. This isn't ideal, but it would keep us from breaking any existing contracts.
There was a problem hiding this comment.
Absolutely, that makes sense, so this looks good to me!
As part of #179 I discovered
oncewas being used to enable sloppy behavior of the request lifecycle management. This PR converts alloncewrappers of callbacks intoonce.strictto ensure proper, valid behavior. This will make further improvements more straightforward.