http: Checking this.connection is not null before using #2172
Conversation
|
Test case? |
|
Unfortunately, I couldn't come up with a test case 😢 I could use some help, if it is absolutely necessary for this change. |
|
Not absolutely necessary - it seems like an edge case - but my train of thought is that if this wasn't caught until now, we have a hole in our tests. |
|
@brendanashworth Another unfortunate thing is, we don't have code coverage reports :'( |
|
LGTM, a test case is helpful but not entirely necessary if it's too difficult to reproduce. Would you be backporting this to v0.12 and v0.10 as well (if necessary)? |
|
@jasnell The corresponding bug in nodejs/node-v0.x-archive#25670 should take care of it, right? |
|
Ah! Yeah, missed that there was an existing PR for that 👍 |
|
@jasnell No problem :-) Thanks for reviewing. @brendanashworth If this LGTY, we can land it. |
|
Yes, LGTM. I'll probably try my hand at a test case later. |
|
@brendanashworth Oh, in that case, you can deliver this fix along with the test, right? Shall I close this then? |
|
@thefourtheye no no, don't let me hold the fix up. This patch is still good. |
Refer: nodejs/node-v0.x-archive#25670 PR-URL: #2172 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brendan Ashworth <[email protected]>
|
Thanks folks :-) Landed at 9afee67 |
Fixes: nodejs/node-v0.x-archive#25670