FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http: remove unneeded cb check from setTimeout by aks- · Pull Request #3631 · nodejs/node · GitHub

/ node Public

http: remove unneeded cb check from setTimeout - #3631

Closed
aks- wants to merge 1 commit into
nodejs:masterfrom
aks-:remove-cb-check-outgoing-message
Closed

http: remove unneeded cb check from setTimeout#3631
aks- wants to merge 1 commit into
nodejs:masterfrom
aks-:remove-cb-check-outgoing-message

Conversation

aks- commented Nov 2, 2015

Copy link
Copy Markdown
Member
  • check is already covered by event emitter

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

Copy link
Copy Markdown
Contributor

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

cc @jasnell who added it in #3090

aks- commented Nov 2, 2015

Copy link
Copy Markdown
Member Author

replaces #3618

Copy link
Copy Markdown
Contributor

Copy link
Copy Markdown
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 commented Nov 3, 2015

Copy link
Copy Markdown
Member

LGTM

aks- commented Nov 4, 2015

Copy link
Copy Markdown
Member Author

should I close this one #3618 ?

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor

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

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

Copy link
Copy Markdown
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. :)

Copy link
Copy Markdown
Contributor

LGTM

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

aks- commented Nov 5, 2015

Copy link
Copy Markdown
Member Author

@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>

Copy link
Copy Markdown
Contributor

Thanks, landed in 8625a38 \o/

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 mentioned this pull request Nov 11, 2015

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
Contributor

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

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL