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 the behavior of Socket.address #50576

Open
brianquinlan opened this issue Nov 29, 2022 · 8 comments
Open

Fix the behavior of Socket.address #50576

brianquinlan opened this issue Nov 29, 2022 · 8 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. breaking-change-approved breaking-change-request This tracks requests for feedback on breaking changes library-io

Comments

@brianquinlan
Copy link
Contributor

brianquinlan commented Nov 29, 2022

Change

Socket.address currently returns the same InternetAddress as Socket.remoteAddress. Instead, it should return the local address that the Socket was bound to.

The original bug is: dart:io, address getter of Socket returns wrong address.

Rationale

The current behavior is not useful. Instead, two connected sockets should have a relationship like:

Expect.equals(clientSocket.address, serverSocket.remoteAddress);
Expect.equals(clientSocket.remoteAddress, serverSocket.address);

There is a work-in-progress change in Gerrit.

Impact

It is hard to know how much existing code this will break. It does not break any Google tests and I would guess that it does not break much existing code.

Mitigation

Users can switch their code to use Socket.remoteAddress instead of Socket.address if they want the remote socket binding.

@brianquinlan brianquinlan added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-io breaking-change-request This tracks requests for feedback on breaking changes labels Nov 29, 2022
@brianquinlan brianquinlan self-assigned this Nov 29, 2022
@itsjustkevin
Copy link
Contributor

@vsmenon @Hixie @grouma we have another breaking change request. Could you take a look?

@grouma
Copy link
Member

grouma commented Dec 5, 2022

Since there is an easy mitigation and no broken Google tests this is probably fine. I do have concerns that some of our developer tools that historically don't have great test coverage could be broken. For example, Dart Dev Runner, Bolt, package:webdev etc.

@Hixie
Copy link
Contributor

Hixie commented Dec 5, 2022

SGTM. Might be worth looking at #12693 at the same time.

@brianquinlan
Copy link
Contributor Author

Ah, I noticed #12693 as well. I think that fixing just that part might be non-breaking but I'll take a look.

@vsmenon
Copy link
Member

vsmenon commented Feb 3, 2023

lgtm

@itsjustkevin
Copy link
Contributor

Marking this as approved.

@itsjustkevin
Copy link
Contributor

@brianquinlan is this work still in flight? Is it planned for Dart 3?

@lrhn lrhn added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. and removed area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. labels Apr 21, 2024
@BBUBBA
Copy link

BBUBBA commented Sep 25, 2024

The bug that happened in 2019, still hasn't been fixed..

Can't P2P UDP HolePunch.

plz.. Update. Thank you !!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. breaking-change-approved breaking-change-request This tracks requests for feedback on breaking changes library-io
Projects
Status: Backlog
Development

No branches or pull requests

7 participants