Skip to content

net: remoteAddress always undefined called before connected#43011

Closed
Y1D7NG wants to merge 1 commit into
nodejs:masterfrom
Y1D7NG:fix-net
Closed

net: remoteAddress always undefined called before connected#43011
Y1D7NG wants to merge 1 commit into
nodejs:masterfrom
Y1D7NG:fix-net

Conversation

@Y1D7NG
Copy link
Copy Markdown
Contributor

@Y1D7NG Y1D7NG commented May 8, 2022

fix #43009

  • when connecting and get remote address, family and port, return this._peername || {} directly.
  • fix remoteFamily is IPvundefined when it doesn't exis, return undefined if it does not exist.
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/net
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem. labels May 8, 2022
Copy link
Copy Markdown
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

@ShogunPanda
Copy link
Copy Markdown
Contributor

Can you link that PR so I can take a look tomorrow?

Copy link
Copy Markdown
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@Y1D7NG
Copy link
Copy Markdown
Contributor Author

Y1D7NG commented May 17, 2022

can this be merged?

@ShogunPanda
Copy link
Copy Markdown
Contributor

At least two approvals and 48 hours passed. I think we're good to go!

@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label May 18, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 18, 2022
Copy link
Copy Markdown
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@ShogunPanda
Copy link
Copy Markdown
Contributor

Landed in b88045f

ShogunPanda pushed a commit that referenced this pull request May 19, 2022
PR-URL: #43011
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@Y1D7NG Y1D7NG deleted the fix-net branch May 19, 2022 14:32
bengl pushed a commit that referenced this pull request May 30, 2022
PR-URL: #43011
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@juanarbol
Copy link
Copy Markdown
Member

This seems to in break v16.x:

make[1]: Leaving directory '/home/juanarbol/GitHub/node/test/node-api/test_worker_terminate/build'                                                       
/usr/bin/python3.10 tools/test.py  --mode=release \                                                                                                      
         \                                                                                                                                               
        --skip-tests= \                                                                                                                                  
        default \                                                                                                                                        
        addons js-native-api node-api                                                                                                                    
=== release test-net-remote-address-port ===                                                                                                             
Path: parallel/test-net-remote-address-port
node:assert:399
    throw err;
    ^

AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:

  assert.ok(remoteFamilyCandidates.includes(socket.remoteFamily))

    at Server.<anonymous> (/home/juanarbol/GitHub/node/test/parallel/test-net-remote-address-port.js:38:10)
    at Server.<anonymous> (/home/juanarbol/GitHub/node/test/common/index.js:417:15)
    at Server.emit (node:events:527:28)
    at TCP.onconnection (node:net:1634:8) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: false,
  expected: true,
  operator: '=='
}

not really sure why, I will label this one as backport-requested in the meantime

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem.

8 participants