Fix send for connected UDP socket

[MSoliankoLuxoft]

In this PR #4270, functionality was added to connect a UDP socket. When we call the SbSocketSendTo function, which invokes sendto for a UDP socket, the issue arises that if this function is called for a socket that was previously connected, it throws an errno 56 (already connected) error on certain platforms, such as macOS or BSD. To address this, we need to call the send function instead of sendto. This behavior is platform-specific, as it works fine on Windows or Linux.

To resolve this issue, I propose a minimal patch that, depending on whether the UDP socket is already connected, will call either sendto or send accordingly. In my opinion, another approach could involve splitting the SbSocketSendTo function into SbSocketSendTo (specifically for non-connected UDP sockets) and SbSocketSend (for TCP and connected UDP sockets). However, this would require significant code changes.

I suggest keeping SbSocketSendTo and handling the connected socket state as I’ve done in this PR. In the future, when we fully transition to the POSIX API, we can directly call the appropriate function (send or sendto) within the UDPSocketStarboard class, instead of using wrappers like SbSocketSendTo. This transition would also create a clearer interface, as currently, when we call SbSocketSendTo for an already connected socket, we pass an address that is ultimately unused. I look forward to hearing your feedback.

[kaidokert]

@jellefoks wdyt ?

It looks like a good attempt at fixing this. From another side though, i wonder if this knowledge or fallback behavior shouldn't be living in a calling code, rather than written in Starboard implementation ?

E.g. we expect this logic to eventually be working with plain send and sendto in Chromium unmodified code, and that difference of behavior between Linux and BSD needs to be handled in that code without Starboards involvement. The calling code will either need to know a priori about the platform behavior, or have a graceful fallback.

[jellefoks]

@jellefoks wdyt ?

It looks like a good attempt at fixing this. From another side though, i wonder if this knowledge or fallback behavior shouldn't be living in a calling code, rather than written in Starboard implementation ?

E.g. we expect this logic to eventually be working with plain send and sendto in Chromium unmodified code, and that difference of behavior between Linux and BSD needs to be handled in that code without Starboards involvement. The calling code will either need to know a priori about the platform behavior, or have a graceful fallback.

The unexpected behavior here is that the upstream posix implementation in UDPSocketPosix::InternalSendTo() always uses sendto.

(Edit): I gave it some more thought, and I think I understand the root cause here. We may need this approach in the interim. I'll elaborate after the weekend.

[jellefoks]

@jellefoks wdyt ?
It looks like a good attempt at fixing this. From another side though, i wonder if this knowledge or fallback behavior shouldn't be living in a calling code, rather than written in Starboard implementation ?
E.g. we expect this logic to eventually be working with plain send and sendto in Chromium unmodified code, and that difference of behavior between Linux and BSD needs to be handled in that code without Starboards involvement. The calling code will either need to know a priori about the platform behavior, or have a graceful fallback.

The unexpected behavior here is that the upstream posix implementation in UDPSocketPosix::InternalSendTo() always uses sendto.

(Edit): I gave it some more thought, and I think I understand the root cause here. We may need this approach in the interim. I'll elaborate after the weekend.

So the root cause is the edge case behavior as described, that the platforms don't behave exactly the same for connected UDP sockets. While connecting UDP sockets theoretically seems to not have a benefit, since UDP is a connectionless protocol, connecting the sockets allows the kernel to do less work per packet, since for connected sockets, the outcome of the packet filtering rule lookup and routing table processing can be memoized and re-used for each subsequent packet.

Originally, Cobalt addressed this by simply not connecting UDP sockets, and therefore avoiding the difference in behavior between the platforms. In addition, a hard assert was added to shared/posix/socket_send_to.cc that would disallow sending of UDP packets without a destination address.

To improve UDP throughput, #4270 was added to allow UDP sockets to be connected, and that PR also removed the hard assert in socket_send_to.cc to allow calling SbSocketSendTo without a destination address on connected sockets. For platforms using a newer version of Cobalt without using an updated version of Starboard, that resulted in the hard assert being triggered, so #4337 was added with a change in udp_socket_starboard.cc to supply a destination address to SbSocketSendTo on connected sockets (unless the platform implemented the g_socket_extension Starboard extension that further improves UDP throughput).

As a result, when udp_socket_starboard.cc is used, platforms using posix/socket_send_to.cc now will supply a destination address to sendto for UDP packets on connected sockets. Linux allows that, but other posix platforms apparently do not.

This PR solves that by remembering when a socket is connected, and (for connected UDP sockets) redirecting to use send instead of sendto. I think that alternatively still using sendto but passing nullptr for the destination will have the same effect.

When Cobalt switches from udp_socket_starboard.cc to udp_socket_posix.cc with Starboard 17, that alternative solution is in use, because the addition from #4337 that supplies a destionation address is no longer active, and also is no longer needed because the hard assert in shared/posix/socket_send_to.cc will no longer have to be avoided since SbSocketSendTo is no longer used.

This change does add a struct member and processing for all sockets, so it could have a negative impact on application performance. On the other hand, there may also be a positive impact on performance when send is used for connected UDP sockets versus sendto with a nullptr destination.

Concluding, there are a few options here:

1. Introduce a (compile time) cobalt configuration flag that strips out the code in udp_socket_starboard.cc that supplies the destination address to SbSocketSendTo (the code at the start of UDPSocketStarboard::InternalSendTo). The SB_HAS_QUIRK pattern seems appropriate here.
2. Use this PR as, if the performance impact of tracking socket connection is negligible, since the issue will go away with posix sockets and Starboard 17.
3. Change this PR to use sendto without a destination address instead of send, and then use as is.
4. If the performance impact of the connection tracking is not negligible, move the shared/posix files changed in this PR to a shared/linux folder to be used for the platforms that allow sendto with a destination address, and use the shared/posix versions only on the platforms that don't allow it.
5. Implement the g_socket_extension on all platforms that don't allow (that means either implementing ReceiveMultiMsg or changing udp_socket_starboard.cc to detect implementations of g_socket_extension that don't implement ReceiveMultiMsg but do signal that the destination address should not be supplied (for example by using the extension version, or allowing ReceiveMultiMsg to be nullptr).

Unless there are any 3p devices that are not linux, I would suggest implementing 1), and setting the flag for platforms that need it.

[kaidokert]

I'd actually try and minimize build-time config of Starboard as much as possible and deprecate the entire concept of QUIRK or such in Starboard 17, i'd consider other options.

[jellefoks]

I'd actually try and minimize build-time config of Starboard as much as possible and deprecate the entire concept of QUIRK or such in Starboard 17, i'd consider other options.

For Starboard 17, this QUIRK can be retired because we will no longer will use udp_socket_starboard.cc where this lives.

[MSoliankoLuxoft]

Perhaps as an alternative, we could revert commit in this PR #4270 since we’ll be migrating to a new version soon that won’t include udp_socket_starboard.cc. In that version, we could add the connected UDP socket implementation, which would make it easier to determine when to call send versus sendto. From what I understand, the udp_socket_posix.cc implementation already exists and might be included in 25.lts—am I correct on that?

[jellefoks]

#4270 gives a large performance boost.

[MSoliankoLuxoft]

Could we please align on the next steps for this PR? Should we proceed with the current fix, or go with one of the solutions suggested by @jellefoks?

[jellefoks]

I think using SB_HAS_QUIRK is the way to go because it should only be an issue on non-linux with BSD derived network stacks.

But instead of using send, have the SB_HAS_QUIRK do a sendto with a nullptr for the address (I think that will also work).

And the reason we need it is an interplay with older Starboard implementations: We connected the UDP socket to improve performance, and since doing that we have to provide an address to sendto if we don't see the g_socket_extension, because of the hard assert in non-updated Starboard, and these network stacks don't allow you to pass in an address when sending on a connected socket.

So on newer Starboard, we won't need it, because we won't be passing an address to sendto on connected sockets already.

Add a define such as SB_HAS_QUIRK_SENDING_CONNECTED_UDP_SOCKETS to the configuration_public.h for the platform, similar to
SB_HAS_QUIRK_NO_CONDATTR_SETCLOCK_SUPPORT in starboard/win/shared/configuration_public.h

And then write the code under the macro, such that the additional processing and memory is only used when #if SB_HAS_QUIRK(SENDING_CONNECTED_UDP_SOCKETS).

It should only be needed temporarily, we should be able to drop that macro for Starboard 17 and up.

[kaidokert]

Okay to go with QUIRK then - @jellefoks i'll leave the actual code review to you