`Socket>>#listenOn:backlogSize:` doesn’t signal an error if another socket is already listening on the same port

[Rinzwind]

In the following example, no error is signaled; shouldn’t the second send of #listenOn:backlogSize: signal an error as the first socket is already listening on the same port?

socket1 := Socket newTCP.
socket1 listenOn: 1701 backlogSize: 1.
socket1 isValid ifFalse: [ Error signal ].
socket2 := Socket newTCP.
socket2 listenOn: 1701 backlogSize: 1.
socket2 isValid ifFalse: [ Error signal ].

Version information:

• Image: Pharo-11.0.0+build.224.sha.6fcd6c1aaf965912b63ffc2ef2059d10cf57c744 (64 Bit)
• VM: Pharo 9.0.18 built on Sep 2 2022 16:54:02 Compiler: 4.2.1 Compatible Apple LLVM 11.0.3 (clang-1103.0.32.29)
• OS: macOS 11.7

[Rinzwind]

I’m not sure I’m looking at the right function in ‘SocketPlugin’, but I see that in ‘sqSocketListenOnPortBacklogSizeInterface’ the return values of the ‘bind’ and ‘listen’ calls on lines 865 and 869 are not used.

[svenvc]

Yes this is an old issue (i.e. it has existed for a long time): when binding/listening no error is signalled when the socket is already in use. This is of course a PITA (see the Zinc issue that you created).

I am sure the plugin code could be much improved and simplified. The challenge remains the cross platform portability though. A developer who could do Linux/macOS is probably ill suited to handle Windows and vice versa.

The same is true for plain and secure networks streams, BTW. Much could be improved/simplified.

[Rinzwind]

This seems to be not consistent across the primitives; quoting issue #11544:

When you try to listen on a port that another process already listens to, in Pharo you only get a primitive failed (primitiveSocketBindToPort) without more details

The error code is accessible using Socket>>sockError which […]

In ‘sqSocketBindToPort’, the return value of the ‘bind’ call does get checked. So in the following example, the second send of #bindTo:port: does signal an error (PrimitiveFailed):

socket3 := Socket newTCP.
socket3 bindTo: NetNameResolver localHostAddress port: 1702.
socket4 := Socket newTCP.
socket4 bindTo: NetNameResolver localHostAddress port: 1702.

And socket4 socketError returns 48 (EADDRINUSE on macOS, see errno.h). In my first example, no error is signaled, and socket2 socketError returns 0.

[MarcusDenker]

11544 has been fixed by merging #11921

[daniels220]

The plugin code could definitely be massively simplified. AFAICT, the entire primSocket:listenOn:backlogSize:interface: family can be removed, replaced by changing the image code to use the existing primSocket:bindTo:port: and primSocket:listenWithBacklog:, both of which do check errors correctly. This way we also get to remove a frankly tremendous amount of duplicated code in both the image (lots of repeated "socket status must be unconnected" checks) and the plugin, including this delightful comment (and an almost identical one above primitiveSocket:listenOnPort:backlogSize:):

/*	one part of the wierdass dual prim primitiveSocketListenOnPort which 
	was warped by some demented evil person determined to twist the very 
	nature of reality */

	/* SocketPlugin>>#primitiveSocket:listenOnPort: */

Does this seem like a good next step, even if there's plenty more to do? If so, I can make a PR for the image side of things, which doesn't require any VM changes to work, just opens the door for cleaning up cruft.

In turn, I'm less than entirely confident I can get all the cruft on the C side—I can try, but is someone willing to help with that?

[Rinzwind]

In the “pharo-vm” repository, there’s an open pull request titled “Feat/improvements in sockets”, I’d suggest to ask there how what you’re proposing could fit in with what they’re working on. Commit 01b21b8963ebcf7c would at least already seem to foil the evil person’s scheme to twist the very nature of reality.

[daniels220]

I submitted a PR implementing the suggestion to use only the separate bind/listen primitives, which should fix this issue if/once accepted.

[guillep]

Hi all,

@daniels220, I believe PR #14593 is good and it should come through in the next couple of hours ;)

@Rinzwind, I talked with Pablo, his PR pharo-project/pharo-vm#604 is a work in progress, so do not hesitate to do another smaller PR, we will take care of the merge.

About the original issue, I acknowledge it is a PITA too, @daniels220 can you confirm that #14593 fixes this issue?

[guillep]

Nevermind, I just saw the comment in the PR :)