remove error-inject and fix error handling
#1409
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1409 +/- ##
=======================================
Coverage 99.38% 99.38%
=======================================
Files 4 4
Lines 484 484
Branches 130 131 +1
=======================================
Hits 481 481
Partials 3 3
Continue to review full report at Codecov.
|
| // overwriting | ||
| if (null != original && original != val) this.remove('Content-Length'); | ||
| if (original != val) { | ||
| val.once('error', err => this.ctx.onerror(err)); |
There was a problem hiding this comment.
The two implementations seem to be not equal. We should only add error event listener when the stream has no other error event listener, which aim to avoid uncaught exception.
Also IMO koa's code philosophy is to keep code simple, so we don't treat 'one-liner module' as a bad thing.
There was a problem hiding this comment.
In current implementation the check in the error-inject does nothing (as two inline created functions never the same) and just always add .on('error') handler. Do you mean that was desired behaviour?
I'm proposing to remove error-inject here not because it's one line, but because even you (module author) forgot that you are breaking it functionality while refactored to inline function. It just does nothing in current version.
There was a problem hiding this comment.
i don't understand what you mean, can you explain in a test?
There was a problem hiding this comment.
@jonathanong Sorry for my English. I've added a failing test to this PR, it's pretty simple:
it('should add error handler to the stream, but only once', () => {
const res = response();
const body = new Stream.PassThrough();
assert.strictEqual(body.listenerCount('error'), 0);
res.body = body;
assert.strictEqual(body.listenerCount('error'), 1);
res.body = body;
assert.strictEqual(body.listenerCount('error'), 1);
});This fails in Koa master and passing after merging this PR.
There was a problem hiding this comment.
seems like a developer error
There was a problem hiding this comment.
It is, but the most probable explanation here is that by hiding the actual logic in a one-line module even the author of the module forgot the logic on a next refactoring and broke the functionality. That's what this PR is all about - fix the error and bring the logic inside to prevent such a problem in the future.
@fengmk2 Why, if there is an error? |
| @@ -108,6 +109,16 @@ describe('res.body=', () => { | |||
| res.body = fs.createReadStream('LICENSE'); | |||
| assert.equal('application/octet-stream', res.header['content-type']); | |||
| }); | |||
|
|
|||
| it('should add error handler to the stream, but only once', () => { | |||
There was a problem hiding this comment.
This test case clearly shows where the problem is, LGTM
The whole
error-injectfunctionality (it's 3 lines actually) is to adderrorevent listener asonlistener, while working around the fact that Node doesn't check if that handler already exists, so, that check was added.However, by obscuring this logic in 3 lines module even module author @dead-horse probably forgot what exactly his module is doing while introducing #800, which basically rendered this check useless by creating inline function.
To prevent this to happen again, and also taking into account that
ctx.onerrorcallsres.endand so,.onceis the more appropriate listener, I'm proposing removingerror-injectdependency and use the fact that respond body assignment already has logic to make something once per stream.