Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: Checking this.connection is not null before using #2172

Closed
wants to merge 1 commit into from

Conversation

thefourtheye
Copy link
Contributor

@Fishrock123 Fishrock123 added the http Issues or PRs related to the http subsystem. label Jul 13, 2015
@brendanashworth
Copy link
Contributor

Test case?

@thefourtheye
Copy link
Contributor Author

Unfortunately, I couldn't come up with a test case 😢 I could use some help, if it is absolutely necessary for this change.

@brendanashworth
Copy link
Contributor

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.

@thefourtheye
Copy link
Contributor Author

@brendanashworth Another unfortunate thing is, we don't have code coverage reports :'(

@jasnell
Copy link
Member

jasnell commented Jul 21, 2015

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)?

@thefourtheye
Copy link
Contributor Author

@jasnell The corresponding bug in nodejs/node-v0.x-archive#25670 should take care of it, right?

@jasnell
Copy link
Member

jasnell commented Jul 21, 2015

Ah! Yeah, missed that there was an existing PR for that 👍

@thefourtheye
Copy link
Contributor Author

@jasnell No problem :-) Thanks for reviewing. @brendanashworth If this LGTY, we can land it.

@brendanashworth
Copy link
Contributor

Yes, LGTM. I'll probably try my hand at a test case later.

@thefourtheye
Copy link
Contributor Author

@brendanashworth Oh, in that case, you can deliver this fix along with the test, right? Shall I close this then?

@brendanashworth
Copy link
Contributor

@thefourtheye no no, don't let me hold the fix up. This patch is still good.

thefourtheye added a commit that referenced this pull request Jul 22, 2015
Refer: nodejs/node-v0.x-archive#25670

PR-URL: #2172
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
@thefourtheye
Copy link
Contributor Author

Thanks folks :-) Landed at 9afee67

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants