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.

Sign up for GitHub

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

Jump to bottom

setTimeout method of net.Socket should return this #32722

Closed
wants to merge 2 commits into from
Closed

setTimeout method of net.Socket should return this #32722

wants to merge 2 commits into from

Conversation

LongTengDao
Copy link
Contributor

@LongTengDao LongTengDao commented Apr 8, 2020 •
edited
Loading

setTimeout method of net.Socket should return this, not undefined, as doc said.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@himself65
Copy link
Member

need a test update for this?

diff --git a/test/parallel/test-net-socket-timeout.js b/test/parallel/test-net-socket-timeout.js
index 8b197b44d6..e01304afe5 100644
--- a/test/parallel/test-net-socket-timeout.js
+++ b/test/parallel/test-net-socket-timeout.js
@@ -70,8 +70,12 @@ for (let i = 0; i < invalidCallbacks.length; i++) {
 const server = net.Server();
 server.listen(0, common.mustCall(() => {
   const socket = net.createConnection(server.address().port);
-  socket.setTimeout(1, common.mustCall(() => {
-    socket.destroy();
-    server.close();
-  }));
+  assert.strictEqual(
+    socket.setTimeout(1, common.mustCall(() => {
+      socket.destroy();
+      assert.strictEqual(socket.setTimeout(1, common.mustNotCall()), socket);
+      server.close();
+    })),
+    socket
+  );
 }));

@LongTengDao
Copy link
Contributor Author

LongTengDao commented Apr 8, 2020 •
edited
Loading

@himself65 Thank you!

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@himself65 himself65 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 10, 2020
@LongTengDao
Copy link
Contributor Author

LongTengDao commented Apr 10, 2020 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/30576/
CI: https://ci.nodejs.org/job/node-test-pull-request/30600/

What's this? Should I do something?

@himself65
Copy link
Member

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

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

What's this? Should I do something?

no, we will land this after ci all green(ci sometimes will crash, I don’t know why)

himself65 pushed a commit to himself65/node that referenced this pull request Apr 11, 2020
@LongTengDao @himself65
lib: fix return type of setTimeout in net.Socket
4439009 
Function setTimeout in net.Socket should return this,
not undefined, as doc said.

PR-URL: nodejs#32722
Refs: https://nodejs.org/api/net.html#net_socket_settimeout_timeout_callback
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@himself65
Copy link
Member

Landed in 4439009

Thanks for your contributions!🎉

@himself65 himself65 closed this Apr 11, 2020
@himself65
Copy link
Member

BTW, I reword the commit message for following the commit rules

targos pushed a commit that referenced this pull request Apr 12, 2020
@LongTengDao @targos
lib: fix return type of setTimeout in net.Socket
23e56ff 
Function setTimeout in net.Socket should return this,
not undefined, as doc said.

PR-URL: #32722
Refs: https://nodejs.org/api/net.html#net_socket_settimeout_timeout_callback
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@targos targos mentioned this pull request Apr 13, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Apr 14, 2020
@LongTengDao @BethGriggs
lib: fix return type of setTimeout in net.Socket
359cabd 
Function setTimeout in net.Socket should return this,
not undefined, as doc said.

PR-URL: #32722
Refs: https://nodejs.org/api/net.html#net_socket_settimeout_timeout_callback
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this pull request Apr 22, 2020
@LongTengDao @targos
lib: fix return type of setTimeout in net.Socket
0db5875 
Function setTimeout in net.Socket should return this,
not undefined, as doc said.

PR-URL: #32722
Refs: https://nodejs.org/api/net.html#net_socket_settimeout_timeout_callback
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@targos targos mentioned this pull request Apr 22, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZYSzys ZYSzys ZYSzys approved these changes

@Flarna Flarna Flarna approved these changes

@trivikr trivikr trivikr approved these changes

@himself65 himself65 himself65 approved these changes

@ronag ronag ronag approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.