Fix UDP send message #4626
Fix UDP send message #4626
Conversation
|
Can you please give a description of this change? I mean, in the PR description please describe what issue is being fixed here (I did not yet look at the code). |
Sorry, I forgot to add a description. |
MSoliankoLuxoft
force-pushed
the
fix_udp_send
branch
from
a544a44 to
14c3767
Compare
This reverts commit e67b550.
MSoliankoLuxoft
force-pushed
the
fix_udp_send
branch
from
14c3767 to
e77e26c
Compare
|
This PR addresses an issue with sending UDP packets for both connected and non-connected sockets. In Cobalt, client sockets are predominantly used, and they are typically connected. As a result, the sendto function is called with nullptr for the sender address, which works correctly on most POSIX-like platforms. However, in Cobalt, there is code that acts as a server. Since the server calls bind, when it sends data, it uses the sendto version that includes the sender’s address. These changes do not affect performance. |
MSoliankoLuxoft
force-pushed
the
fix_udp_send
branch
from
e77e26c to
8b6c8ab
Compare
| // Otherwise, if remote_address_ is not set, we use the 'address' parameter | ||
| // to specify the destination for the data. | ||
| if (remote_address_) { | ||
| int result = SbSocketSendTo(socket_, buf->data(), buf_len, nullptr); |
There was a problem hiding this comment.
There is actually one more edge case.
Older implementations of Starboard always require an address to be passed for UDP sockets into SbSocketSendTo (*1). The SB_LOG(FATAL) actually results in a runtime assert (crash) if we don't pass in an address. Unfortunately, it was not implemented to simply return a failure instead.
Removing the address actually results in a performance boost as a result of less time spent in the kernel syscall, even on connected sockets, so we prefer to not pass in the address, even though ignoring performance, it's technically fine to do so.
We can't know if the implementation of SbSocketSendTo on a platform still has the address check, so we have to assume that it needs an address unless we otherwise know that we can leave it out. I used the existence of the socket extension for reading multiple UDP packets in a single call ( - which was added for performance - ) as the signal that the platfom has been updated. The assumption is that when a platform is updated to implement the extension, it also can (and should) update the SbSocketSendTo implementation.
- That means that there has to be a
address = remote_address_.get();in combination with anif (!g_socket_extension)sprinkled around here somewhere to ensure that on platforms where(!g_socket_extension), we always pass in an address in calls toSbSocketSendTo, and for connected sockets that means we have to get the address fromremote_address_because Cobalt now doesn't give us the address as the parameter anymore.
*1: https://github.com/youtube/cobalt/blob/24.lts.1%2B/starboard/shared/posix/socket_send_to.cc#L61
There was a problem hiding this comment.
Thank you for the feedback! You are absolutely right, and to properly implement the use of g_socket_extension, we need to add support for this feature on certain platforms. Unfortunately, this extension is not implemented on most platforms at the moment, so it will need to be added and thoroughly tested to ensure there are no issues.
Specifically, if g_socket_extension == nullptr, it will result in an error when calling SbSocketSendTo for a connected socket on Linux, as older implementations still require an address. Therefore, additional time is needed to implement this approach and verify that it works correctly across all required platforms.
There was a problem hiding this comment.
@jellefoks
Adding a check for if (g_socket_extension) introduces an issue because some platforms do not support the multi-packet sending extension, similar to how recvmmsg works on Linux. As a result, g_socket_extension will be nullptr in some cases, meaning it does not always indicate whether the platform has been updated.
For example, macOS does not support this functionality, and due to the current logic, we would assume the platform is not updated and pass an address to SbSocketSendTo. However, this leads to an error when calling sendto on a connected socket.
To address this, I decided to use a macro that explicitly indicates whether the platform has been updated. This allows us to safely call SbSocketSendTo with nullptr as the destination address for a connected socket. For a platform to support this behavior, it needs to define this macro and also update its SbSocketSendTo implementation (removing the address check if necessary).
This is a temporary solution, but it resolves the issue on Apple platforms, where the current behavior does not work when using a UDP server that replies to a specific destination (i.e., an unconnected socket).
MSoliankoLuxoft
force-pushed
the
fix_udp_send
branch
2 times, most recently
from
d0a52ec to
9c20a9d
Compare
This commit fixes UDP packet sending for both connected and unconnected sockets.
b\383980995
Change-Id: I2aff8facdc676233e9dac10259cdbe3124a984a6
MSoliankoLuxoft
force-pushed
the
fix_udp_send
branch
from
9c20a9d to
3a9534d
Compare
This PR fixes UDP packet sending for both connected and unconnected sockets, as the code includes a UDP server that calls sendto for an unconnected UDP socket.
b/383980995