dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.28k stars 1.58k forks source link

Add remote port information to Socket errors #12693

Open whesse opened 11 years ago

whesse commented 11 years ago

Errors thrown by Socket.connect currently print the local host and local port information, rather than the remote host and port.

To fix this, we would need to print both, since in the case of a socket created by accept on a listening socket, we want to know the local host and port, and there is no difference between a socket created by listening and one created by connect.

The _NativeSocket class has a cached copy of the local port, but the remote port information can only be gotten from the nativeGetRemotePeer call, which will fail if a connection has failed, like by connection refused.

So to implement this, we would need a remote port field in NativeSocket, which is filled in when a connect attempt is made, or when an accept is performed. Then we could report this in errors.

andersjohnsen commented 11 years ago

Set owner to @skabet.

sgjesse commented 11 years ago

Removed the owner.

sgjesse commented 11 years ago

Added this to the Later milestone.

sgjesse commented 11 years ago

Issue #14783 has been merged into this issue.

andersjohnsen commented 11 years ago

I'll look into sorting this out, making it less confusing.


Set owner to @skabet. Added Started label.

kevmoo commented 10 years ago

Removed Area-IO label. Added Library-IO, Area-Library labels.

andersjohnsen commented 10 years ago

Removed the owner. Added Triaged label.

kasperl commented 10 years ago

Removed this from the Later milestone. Added Oldschool-Milestone-Later label.

kasperl commented 10 years ago

Removed Oldschool-Milestone-Later label.

Hixie commented 7 years ago

It seems that for some of the messages, it's actually the local port and the remote host name, which is a significant source of confusion when debugging socket-based communications. (For a while I thought my program had a bug where I was setting the wrong remote port.)

mleonhard commented 5 years ago

I just wasted an hour because of this issue. How about bumping its priority?

natebosch commented 5 years ago

@sortie - this has come up a few times recently.

georgeherby commented 5 years ago

Another duplicate issue with more details in it too #28753

sortie commented 5 years ago

Thanks for triaging duplicate issues. This bug is a favorite pet peeve of mine. I'm working on a fix as part of a breaking change.

ulidtko commented 5 years ago

Well... being reported in 2013, almost 6 years ago, this bug is graduating into a pet dragon peeve I guess.

For anyone having the capacity to use their own build of Dart SDK, PR #36038 (unmerged) has a single-line tweak that you can cherry-pick and get the correct port in logs (for client sockets at least). Without any breaking changes.

Kaushal0709 commented 5 years ago

Well... being reported in 2013, almost 6 years ago, this bug is graduating into a pet dragon peeve I guess.

For anyone having the capacity to use their own build of Dart SDK, PR #36038 (unmerged) has a single-line tweak that you can cherry-pick and get the correct port in logs (for client sockets at least). Without any breaking changes.

We are also facing the same problem in flutter 2.4.0. Can you please help me to resolved this if it is only one line fix.

Regards, Kaushal

ulidtko commented 5 years ago

@Kaushal0709,

We are also facing the same problem in flutter 2.4.0. Can you please help me to resolved this if it is only one line fix.

It's here: https://github.com/dart-lang/sdk/pull/36038/commits/2acc8a415b046e5722ab28d8f4276fff90d62252

You will need to cherry-pick that commit into your Dart SDK source (or just edit in that line manually), then build Dart SDK from the patched source, and use that in your Flutter distribution.

The process is exactly the same as building Dart SDK from source, except that you do one extra step between Getting the source and Building, namely: patching the source.

sortie commented 4 years ago

I have a proposed fix at https://dart-review.googlesource.com/c/sdk/+/127465.

ulidtko commented 4 years ago

Rebased patch for 2.7.0, until the breaking change lands into a release:

From: Max <ulidtko@gmail.com>
Date: Thu, 19 Dec 2019 16:19:07 +0000
Subject: [PATCH] Fix wrong port in Dart SocketError messages

See https://github.com/dart-lang/sdk/issues/12693
and https://github.com/dart-lang/sdk/pull/36038

Apply to: dart-sdk 2.7.0 (git 4cc36055d6803b899667feaedc1216a96e9d1c72)

---
 runtime/bin/socket_patch.dart | 1 +
 1 file changed, 1 insertion(+)

diff --git c/sdk/lib/_internal/vm/bin/socket_patch.dart i/sdk/lib/_internal/vm/bin/socket_patch.dart
index 5771c76909..c0b9041b4f 100644
--- c/sdk/lib/_internal/vm/bin/socket_patch.dart
+++ i/sdk/lib/_internal/vm/bin/socket_patch.dart
@@ -499,6 +499,7 @@ class _NativeSocket extends _NativeSocketNativeWrapper with _ServiceObject {
         final _InternetAddress address = it.current;
         var socket = new _NativeSocket.normal();
         socket.localAddress = address;
+        socket.localPort = port;
         var result;
         if (sourceAddress == null) {
           result = socket.nativeCreateConnect(
sortie commented 4 years ago

The above patch is not correct as the problem is deeper (the whole localAddress and localPort variable semantics were wrong) and IIRC might actually cause other issues. If one is willing to cherry-pick a patch anyway, one can apply: https://dart-review.googlesource.com/c/sdk/+/127465

I'm currently quantifying how potentially breaking the fix is (changing the semantics of .address, the port fix itself is non-breaking, but I bundled them together) (it's probably not very breaking, but we are careful). I've done a semantic analysis of all pub packages and need to review the results. I also need to review other internal source code likewise. I'll do that once I'm back in the new year, and then I'll proceed to file a proper breaking change request, fix a couple tests, and get the change landed.

AbeerSul commented 4 years ago

none of this actually worked for me :/ this command worked like magic "adb reverse tcp:5000 tcp:5000" 💯

Kushagrasri commented 4 years ago

i tried everything, working on emulator, android, but the same issue appeared all the time. even tried adb reverse tcp:port tcp:port then i used an application called ngrok which redirects to your localhost:port. still, i was getting the same error. then, i disabled windows firewall and eureka!!!! hope this helps someone.

davidmorgan commented 3 years ago

+1 to the wrong (local) port being shown on connection failures--this cost us some engineering time this week.

Good luck on the fix ;) from my point of view, not too worried about wrong semantics that have been there for a long time--just the port in the error message would have saved us. Thanks.

a-siva commented 3 years ago

@sortie do you have cycles to take your CL thru the review and breaking change process and land it or do you want somebody else to takeover and work on it ?

matheszabi commented 8 months ago

It is still not fixed!!! Why???

final channel = ClientChannel('localhost',
        port: 8080,
        options:
        const ChannelOptions(credentials: ChannelCredentials.insecure()));

Caught error: gRPC Error (code: 14, codeName: UNAVAILABLE, message: Error connecting: SocketException: Connection refused (OS Error: Connection refused, errno = 111), address = localhost, port = 37674, details: null, rawResponse: null, trailers: {})

The 37674 is randomly changing

I have no idea why can't get to port 8080, since:


    await server.serve(port: 8080);
    print('Server listening on port ${server.port}...');

Server listening on port 8080...

a-siva commented 8 months ago

@brianquinlan could we take over this CL https://dart-review.googlesource.com/c/sdk/+/127465 and take it through the breaking change process and land it.

ulidtko commented 8 months ago

... the amusing story of counting the years it takes Google to merge a single-line patch :hear_no_evil:

brianquinlan commented 8 months ago

I had a PR to do this: https://dart-review.googlesource.com/c/sdk/+/272520

But I couldn't get the windows tests to pass. Let me take a look at it again.

brianquinlan commented 8 months ago

@matheszabi please follow the Dart Code of Conduct and behave respectfully.

matheszabi commented 8 months ago

@matheszabi please follow the Dart Code of Conduct and behave respectfully.

GOOGLE should fire more employees, especially teachers, who are in coding positions, because it would be less bugs in code and even less useless comments, more time for bug fixing, which is needed to correct the weak work of teachers!