Skip to content

net: fix usage of writeBuffer in makeSyncWrite - #19103

Closed
joyeecheung wants to merge 2 commits into
nodejs:masterfrom
joyeecheung:fix-win-write-buffer
Closed

net: fix usage of writeBuffer in makeSyncWrite#19103
joyeecheung wants to merge 2 commits into
nodejs:masterfrom
joyeecheung:fix-win-write-buffer

Conversation

@joyeecheung

Copy link
Copy Markdown
Member

The binding writeBuffer has been changed in
#19041 and it now requires
the last argument to be a context object. makeSyncWrite
was not updated accordingly, resulting assertions on Windows.
This patch fixes the usage of writeBuffer there.

Refs: #19041

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

net

The binding writeBuffer has been changed in
nodejs#19041 and it now requires
the last argument to be a context object. makeSyncWrite
was not updated accordingly, resulting assertions on Windows.
This patch fixes the usage of writeBuffer there.
@nodejs-github-botnodejs-github-bot added the net Issues and PRs related to the net subsystem. label Mar 3, 2018
@joyeecheungjoyeecheung added the fast-track PRs that do not need to wait for 48 hours to land. label Mar 3, 2018
@joyeecheung

Copy link
Copy Markdown
MemberAuthor

cc @addaleax

Adding fast-track to unbreak Windows CI on master

@joyeecheung

Copy link
Copy Markdown
MemberAuthor

@joyeecheung
joyeecheungforce-pushed the fix-win-write-buffer branch from 8d18e3b to 9620e7bCompareMarch 3, 2018 09:16
@joyeecheung

Copy link
Copy Markdown
MemberAuthor

Updated to fix the enumerable error.message showing up in test-stdout-close-catch's deepStrictEqual. @addaleax PTAL, thanks!

New CI: https://ci.nodejs.org/job/node-test-pull-request/13466/

@addaleaxaddaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM :)

@joyeecheung

Copy link
Copy Markdown
MemberAuthor

Only one known failure in Windows, fix is in #19093

I'll go ahead and land this to unbreak master.

@joyeecheung

Copy link
Copy Markdown
MemberAuthor

Landed in 67b5985, thanks!

joyeecheung added a commit that referenced this pull request Mar 3, 2018
The binding writeBuffer has been changed in
#19041 and it now requires
the last argument to be a context object. makeSyncWrite
was not updated accordingly, resulting assertions on Windows.
This patch fixes the usage of writeBuffer there.
Also fix errors.uvException() so error.message are no longer
enumerable, this fixes the deepStrictEqual assertion on the
error object in test-stdout-close-catch.
PR-URL: #19103
Refs: #19041
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
The binding writeBuffer has been changed in
nodejs#19041 and it now requires
the last argument to be a context object. makeSyncWrite
was not updated accordingly, resulting assertions on Windows.
This patch fixes the usage of writeBuffer there.
Also fix errors.uvException() so error.message are no longer
enumerable, this fixes the deepStrictEqual assertion on the
error object in test-stdout-close-catch.
PR-URL: nodejs#19103
Refs: nodejs#19041
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fast-trackPRs that do not need to wait for 48 hours to land.netIssues and PRs related to the net subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@joyeecheung@addaleax@nodejs-github-bot