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.

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

Fix connect socket InetSocketAddress #3597

Closed

Conversation

yangbodong22011
Copy link
Collaborator

Passing host directly will avoid another call to InetAddress.getByName inside the InetSocketAddress function.

For machines with ipv4 and ipv6, but the startNode uses ipv4 to connect, the ipv6 connection may fail.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d2f6712) 71.50% compared to head (73a4469) 71.46%.

❗ Current head 73a4469 differs from pull request most recent head d9fb789. Consider uploading reports for the commit d9fb789 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3597      +/-   ##
============================================
- Coverage     71.50%   71.46%   -0.04%     
+ Complexity     4851     4847       -4     
============================================
  Files           288      288              
  Lines         15461    15461              
  Branches       1095     1095              
============================================
- Hits          11055    11049       -6     
- Misses         3930     3934       +4     
- Partials        476      478       +2     
Files Coverage Δ
...redis/clients/jedis/DefaultJedisSocketFactory.java 96.05% <100.00%> (ø)

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sazzad16
Copy link
Collaborator

@yangbodong22011 The code makes sense.

But to make sure your reasoning, do you understand that due to

    List<InetAddress> hosts = Arrays.asList(InetAddress.getAllByName(hostAndPort.getHost()));
    if (hosts.size() > 1) {
      Collections.shuffle(hosts);
    }

you can't ensure ipv4 comes before ipv6?

@yangbodong22011
Copy link
Collaborator Author

@yangbodong22011 The code makes sense.

But to make sure your reasoning, do you understand that due to

    List<InetAddress> hosts = Arrays.asList(InetAddress.getAllByName(hostAndPort.getHost()));
    if (hosts.size() > 1) {
      Collections.shuffle(hosts);
    }

you can't ensure ipv4 comes before ipv6?

Yes, I noticed the shuffle.

My scenario is that startNode is localhost, and two InetAddress are obtained through InetAddress.getAllByName, one is ipv4 and the other is ipv6 (my machine has two IPs at the same time).

During the subsequent connection process, if it is an ipv6 address, after socket.connect, an error "Unexpected end of stream." will be reported when performing read.

Because of the old code, the ipv6 address will still perform getAllByName in new InetSocketAddress. At this time, the host obtained is Null, which may cause read to return eof.

image (The left is the new code, the right is the old code)

@yangbodong22011
Copy link
Collaborator Author

@sazzad16 I'd prefer to merge this PR and let #3596 rebase it.

This will provide more information for people looking at the code in the future, at least #3597 this link.

@sazzad16 sazzad16 self-requested a review November 2, 2023 11:51
@sazzad16
Copy link
Collaborator

sazzad16 commented Nov 2, 2023

I'd prefer to merge this PR and let #3596 rebase it.

This will provide more information for people looking at the code in the future, at least #3597 this link.

@yangbodong22011 I'm not sure. This change was not necessary until #3596.
Moreover if we separate the changes, #3596 won't be self sufficient. I'd assume we'd have a 4.5.0 release including that PR. In that case there would be an added liability to downmerge this PR in 4.x branch while downmerging that #3596.
IMO, adding code comment there is lesser hectic way.

@yangbodong22011
Copy link
Collaborator Author

@sazzad16 ok, I noticed that you have added a comment in #3596, then let us abandon this PR and move to 3596.

@sazzad16
Copy link
Collaborator

sazzad16 commented Nov 2, 2023

Resolved in #3596

@sazzad16 sazzad16 closed this Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants