Skip to content

Fix Socket remoteAddress obtaining on Win32 #51778 #51981

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

Closed
wants to merge 7 commits into from

Conversation

slonm
Copy link
Contributor

@slonm slonm commented Apr 7, 2023

Looks getpeername() is broken when it used after AcceptEx(). I found a couple mentions of such issue but it's strange that they are pretty old:

https://www.codeproject.com/Messages/540371/Re-Win2K-AcceptEx-amp-amp-getpeername
https://alt.winsock.programming.narkive.com/ZUX8KVbu/so-update-accept-context-usage

I've implemented the workaround and call GetAcceptExSockaddrs() instead of getpeername() immediately after socket connection accept.

Now Unit test tests\standalone_2\io\socket_info_ipv6_test.dart is passed without errors.

@slonm
Copy link
Contributor Author

slonm commented Apr 7, 2023

Initial issue #51778

@copybara-service
Copy link

Thank you for your contribution. This project uses Gerrit for code reviews. Your pull request has automatically been converted into a code review at:

https://dart-review.googlesource.com/c/sdk/+/294220

Please wait for a developer to review your code review at the above link. See CONTRIBUTING.md to learn how to upload changes to Gerrit directly. You can speed up the review if you sign into Gerrit and manually add a reviewer that has recently worked on the relevant code and they will help you. You can also push additional commits to this pull request to update the code review.

@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/294220 has been updated with the latest commits from this pull request.

@mit-mit
Copy link
Member

mit-mit commented Apr 17, 2023

Hi @slonm thanks for the contribution; can you please rebase it?

@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/294220 has been updated with the latest commits from this pull request.

1 similar comment
@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/294220 has been updated with the latest commits from this pull request.

@slonm
Copy link
Contributor Author

slonm commented Apr 17, 2023

Hi @mit-mit, I've resolved the conflict but not sure whether I've done things right regarding rebasing. Let me know if there is something wrong

@mit-mit
Copy link
Member

mit-mit commented Apr 17, 2023

Thanks. Stay tuned for comments in the dart-review link.

@brianquinlan
Copy link
Contributor

@slonm There are comments at https://dart-review.googlesource.com/c/sdk/+/294220

@slonm
Copy link
Contributor Author

slonm commented Apr 26, 2023

@brianquinlan - should I do something with it?

@brianquinlan
Copy link
Contributor

@slonm That's up to you. There are two ways that we can go here:

  1. You address the comments in the code review and, after we reach a consensus, we commit your change.
  2. We abandon your pull request, I recreate it and then send it to another Dart developer for review.

Let me know which you'd prefer.

@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/294220 has been updated with the latest commits from this pull request.

1 similar comment
@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/294220 has been updated with the latest commits from this pull request.

@slonm
Copy link
Contributor Author

slonm commented Apr 27, 2023

Performed code review. Agreed with most of suggestions

@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/294220 has been updated with the latest commits from this pull request.

1 similar comment
@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/294220 has been updated with the latest commits from this pull request.

@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/294220 has been updated with the latest commits from this pull request.

1 similar comment
@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/294220 has been updated with the latest commits from this pull request.

@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/294220 has been updated with the latest commits from this pull request.

1 similar comment
@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/294220 has been updated with the latest commits from this pull request.

@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/294220 has been updated with the latest commits from this pull request.

1 similar comment
@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/294220 has been updated with the latest commits from this pull request.

@copybara-service copybara-service bot closed this in a4b2502 Jun 7, 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