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: remove unneeded cb check from setTimeout #3631

Closed
wants to merge 1 commit into from

Conversation

aks-
Copy link
Member

@aks- aks- commented Nov 2, 2015

  • check is already covered by event emitter

@Fishrock123 Fishrock123 added the http Issues or PRs related to the http subsystem. label Nov 2, 2015
@Fishrock123
Copy link
Contributor

LGTM, this retains the same functionality since EventEmitter catches it and also throws.

cc @jasnell who added it in #3090

@aks-
Copy link
Member Author

aks- commented Nov 2, 2015

replaces #3618

@Fishrock123
Copy link
Contributor

@Fishrock123
Copy link
Contributor

https://ci.nodejs.org/job/node-test-binary-windows/185/RUN_SUBSET=2,VS_VERSION=vs2013,label=win2008r2/tapTestReport/test.tap-11/

not ok 11 test-child-process-fork-regr-gh-2847.js
# events.js:141
# throw er; // Unhandled 'error' event
# ^
# 
# Error: read ECONNRESET
# at exports._errnoException (util.js:860:11)
# at TCP.onread (net.js:544:26)

Looks unrelated but confirmation would be great, I'm not familiar with that test.

@jasnell
Copy link
Member

jasnell commented Nov 3, 2015

LGTM

@aks-
Copy link
Member Author

aks- commented Nov 4, 2015

should I close this one #3618 ?

@jbergstroem
Copy link
Member

@Fishrock123 I've seen that elsewhere recently too. Could you open a new issue?

@Fishrock123
Copy link
Contributor

@aks- Yes you can close the older PR. :)

Edit: I'll just do it when I merge this.

@Fishrock123
Copy link
Contributor

@aks- Your git is setup to sign commits as aks- <[email protected]>, usually we use non-usernames there, if you would like that and/or be comfortable with it. :)

It's not required, so if you are fine with it being aks- just let me know.,

You can change it by doing:

git config --global user.name "Jeremiah Senkpiel"

git commit --amend --reset-author --no-edit

And force pushing the new commit back up here. :)

@thefourtheye
Copy link
Contributor

LGTM

- check is already covered by event emitter
@aks- aks- force-pushed the remove-cb-check-outgoing-message branch from a1f7e1c to 395e324 Compare November 5, 2015 14:33
@aks-
Copy link
Member Author

aks- commented Nov 5, 2015

@Fishrock123 done :)

Fishrock123 pushed a commit that referenced this pull request Nov 5, 2015
- This check is already covered in EventEmitter#addListener()

Refs: #3618
PR-URL: #3631
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@Fishrock123
Copy link
Contributor

Thanks, landed in 8625a38 \o/

@Fishrock123 Fishrock123 closed this Nov 5, 2015
rvagg pushed a commit that referenced this pull request Nov 7, 2015
- This check is already covered in EventEmitter#addListener()

Refs: #3618
PR-URL: #3631
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@Fishrock123 Fishrock123 mentioned this pull request Nov 11, 2015
@MylesBorins
Copy link
Contributor

this was originally proposed to roll back changes found in 0094a8d

The associated PR #3090 is semver-major. As such I am removing the label for lts watch.

@MylesBorins
Copy link
Contributor

@jasnell removing lts-watch as per previous comment. These changes only apply to master... not v4.x-staging

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.

7 participants