lib: refactor Socket._getpeername and Socket._getsockname by himself65 · Pull Request #32969 · nodejs/node

himself65 · 2020-04-21T12:04:10Z

Refs:

Lines 79 to 85 in 7893c70

    
           sockaddr_storage storage; 
        
           int addrlen = sizeof(storage); 
        
           sockaddr* const addr = reinterpret_cast<sockaddr*>(&storage); 
        
           const int err = F(&wrap->handle_, addr, &addrlen); 
        
           if (err == 0) 
        
             AddressToJS(wrap->env(), addr, args[0].As<v8::Object>()); 
        
           args.GetReturnValue().Set(err);

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

lib/net.js

nodejs-github-bot · 2020-05-24T14:13:48Z

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

nodejs-github-bot · 2020-05-24T16:31:14Z

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

bnoordhuis · 2020-05-26T11:18:18Z

lib/net.js

+  if (!this._handle || !this._handle.getpeername) {
+    return this._peername || {};
+  } else if (!this._peername) {
+    this._peername = {};


This is a functional change in that before errors can be transient, i.e., the first call can return an empty object while the second call can return a full-fledged address object. Now errors are no longer transient because the empty object is cached.

It's a probably inconsequential change in behavior but I'd label it semver-major anyway.

lib/net.js

Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl>

nodejs-github-bot · 2020-05-29T09:51:06Z

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

jasnell · 2020-05-31T00:07:49Z

Marking semver-major per #32969 (comment)

jasnell · 2020-06-19T03:39:54Z

@nodejs/tsc ... since this is semver-major we need another TSC signoff

jasnell · 2020-06-24T23:27:38Z

Removed the author ready because this needs at least one more tsc signoff in order to land

mcollina

lgtm

github-actions · 2020-10-17T00:38:01Z

Commit Queue failed

- Loading data for nodejs/node/pull/32969
✔  Done loading data for nodejs/node/pull/32969
----------------------------------- PR info ------------------------------------
Title      lib: refactor Socket._getpeername and Socket._getsockname (#32969)
Author     Himself65  (@Himself65)
Branch     Himself65:20200421-address -> nodejs:master
Labels     author ready, net, semver-major
Commits    5
 - lib: refactor Socket._getpeername and Socket._getsockname
 - fixup!
 - fixup! backport
 - fixup!
 - fixup! edit comment
Committers 2
 - himself65 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/32969
Refs: https://github.com/nodejs/node/blob/7893c70970adfbefb1684c48d42aff7385a2afb8/src/node_internals.h#L79-L85
Reviewed-By: James M Snell 
Reviewed-By: Ben Noordhuis 
Reviewed-By: Matteo Collina 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/32969
Refs: https://github.com/nodejs/node/blob/7893c70970adfbefb1684c48d42aff7385a2afb8/src/node_internals.h#L79-L85
Reviewed-By: James M Snell 
Reviewed-By: Ben Noordhuis 
Reviewed-By: Matteo Collina 
--------------------------------------------------------------------------------
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2020-10-16T23:28:31Z: https://ci.nodejs.org/job/node-test-pull-request/33684/
- Querying data for job/node-test-pull-request/33684/
✔  Build data downloaded
- Querying failures of job/node-test-commit/41438/
✔  Data downloaded
   ✖  2 failure(s) on the last Jenkins CI run
   ℹ  This PR was created on Tue, 21 Apr 2020 12:04:10 GMT
   ✔  Approvals: 3
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/32969#pullrequestreview-417884204
   ✔  - Ben Noordhuis (@bnoordhuis): https://github.com/nodejs/node/pull/32969#pullrequestreview-418200196
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/32969#pullrequestreview-464801397
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

Commit Queue action: https://github.com/nodejs/node/actions/runs/311694300

nodejs-github-bot · 2020-10-19T16:34:08Z

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

aduh95 · 2020-10-19T18:36:09Z

Landed in 1428db8

PR-URL: #32969 Refs: https://github.com/nodejs/node/blob/7893c70970adfbefb1684c48d42aff7385a2afb8/src/node_internals.h#L79-L85 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Apr 21, 2020

addaleax reviewed Apr 27, 2020

View reviewed changes

lib/net.js Outdated Show resolved Hide resolved

BridgeAR reviewed May 23, 2020

View reviewed changes

lib/net.js Outdated Show resolved Hide resolved

lib: refactor Socket._getpeername and Socket._getsockname

7d4d597

himself65 force-pushed the 20200421-address branch from 5008754 to 7d4d597 Compare May 24, 2020 13:54

BridgeAR reviewed May 24, 2020

View reviewed changes

lib/net.js Outdated Show resolved Hide resolved

fixup!

f61cae2

BridgeAR requested a review from bnoordhuis May 24, 2020 14:35

fixup! backport

faa57e8

jasnell approved these changes May 25, 2020

View reviewed changes

bnoordhuis approved these changes May 26, 2020

View reviewed changes

himself65 and others added 2 commits May 29, 2020 17:43

fixup!

0156c9c

Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl>

fixup! edit comment

8adc336

himself65 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 30, 2020

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 31, 2020

BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18

jasnell removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 24, 2020

himself65 requested a review from a team as a code owner August 10, 2020 16:07

himself65 requested a review from a team August 10, 2020 16:07

mcollina approved these changes Aug 11, 2020

View reviewed changes

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 16, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 16, 2020

This comment has been minimized.

Sign in to view

github-actions bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 17, 2020

aduh95 closed this Oct 19, 2020

targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021

	sockaddr_storage storage;
	int addrlen = sizeof(storage);
	sockaddr* const addr = reinterpret_cast<sockaddr*>(&storage);
	const int err = F(&wrap->handle_, addr, &addrlen);
	if (err == 0)
	AddressToJS(wrap->env(), addr, args[0].As<v8::Object>());
	args.GetReturnValue().Set(err);

Uh oh!

Conversation

himself65 commented Apr 21, 2020

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented May 24, 2020

Uh oh!

nodejs-github-bot commented May 24, 2020

Uh oh!

bnoordhuis May 26, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented May 29, 2020

Uh oh!

jasnell commented May 31, 2020

Uh oh!

jasnell commented Jun 19, 2020

Uh oh!

jasnell commented Jun 24, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Oct 17, 2020

Uh oh!

nodejs-github-bot commented Oct 19, 2020

Uh oh!

aduh95 commented Oct 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants