Socket bugs

Hi,
I found several bug and typo on DatagramSocket.


First one when bind it and shutdown DatagramSocket multiple times:

The following snippet work perfectly well:

std::unique_ptr<DatagramSocket> datagramSocket(new DatagramSocket());
bool res = datagramSocket->bindToPort(12345, "127.0.0.1")
jassert(res); // ok
datagramSocket->shutdown(); // useless because performed by ~DatagramSocket on next reset()
datagramSocket.reset(new DatagramSocket());
bool res = datagramSocket->bindToPort(12345, "127.0.0.1")
jassert(res); // ok
datagramSocket->shutdown(); // same remark than upper
datagramSocket.reset();

but the following snippet fail:

DatagramSocket datagramSocket;
bool res = datagramSocket.bindToPort(12345, "127.0.0.1")
jassert(res); // ok
datagramSocket.shutdown();
bool res = datagramSocket.bindToPort(12345, "127.0.0.1")
jassert(res); // !!! FAILURE !!!
datagramSocket.shutdown();

This is because DatagramSocket create the handle and shutdown() and ~DatagramSocket destroy the handle and bindToPort is unable to bind without an handle.

I fix that by letting the handle be recreated by bindToPort if it was previously destroyed
and set isBound at false in DatagramSocket::shutdown()


Note that StreamingSocket::connect and StreamingSocket::close() abusively call system call SocketHelpers::closeSocket when not needed. Letting underlying ::closesocket system call fail.


static const SocketHandle invalidSocket; is a very good cross platform design but oftenly not used.

On Win32 invalidSocket is INVALID_SOCKET aka (SOCKET)(~0).
on other plateform it is -1.
So pretty the same value in fact.

Note that ::socket() system call could return 0 as a valid handle value (cf MSDN or unix man page).

When reading the Juce socket code, I found a lot of heterogeneous way to check socket handle validity:

Some are bad:
handle > 0

Some could be better

h != (unsigned) SOCKET_ERROR
handle >= 0
handle < 0
handle != -1


Here a patch of the current develop branch of Juce git to fix all of this:

http://download.smode.fr/forum/juceSocketFixes.patch

Hopping all of this can helps.

Bump,

Hi Juce team!
Nobody is interested to merge this quick fix?
Or should I reformulate the rational?

Thanks in advance for this.
All the best

Thanks for the patch and sorry for the delay in getting back to you. I’ve pushed a change to the develop branch here which integrates some of these changes, namely allowing socket handles of 0. As for the other change that you proposed which would allow DatagramSocket objects to be re-used after calling shutdown(), the documentation for this method says:

“Note that all other methods will return an error after this call.”

so I think the intended behaviour is that you can’t re-use one of these objects after shutting down and you should use the approach in your first code snippet.

1 Like

Hi Ed,

Many thanks for your feedback, and your reactivity!
Thanks for the zero handle conformance and the closeSocket cleanup.
I’m sure you’ve got a lot of works. I do not want to hurry you.
I just wanted to know if my post will be borrowed at the bottom of the forum or not…

StreamingSocket can be reused after a close, so that why I try to promote this behaviour for DatagramSocket, this to enhance Juce API consistency.
This surprising pitfall introduce a bug on my software. Of course I should RTFM!

Thanks for your concern about this.
All the best