Skip to content

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

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- <coderatlabs@gmail.com>, 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 <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@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 <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@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.

7 participants