rtspconnection: Add API for sending multiple messages at once, and for having the message body consist of multiple chunks of memory
Submitted by anila
Link to original bug (#785684)
Description
An API to write messages to the connection without blocking.
This is need for the support for buffer lists in gst-rtsp-server in the tcp case.MSG_DONTWAIT flag is send to sendmsg () so that nonblocking write of messages are possible.
Blocking
Activity
Patch 356721, "api to write messages to rtsp connection without blocking":
0001-Api-to-write-messages-to-rtspconnection-without-bloc.patch
@ocretereviewed patch 356721:::: gst-libs/gst/rtsp/gstrtspconnection.c @@ +1478,3 @@ + num_written = + g_socket_send_message (socket, NULL, &vecs[i], send_len, NULL, 0, + MSG_DONTWAIT, NULL, &err);MSG_DONTWAIT doesn'T exist on Windows. You can't do it like that. I think you want:
https://developer.gnome.org/gio/stable/GDatagramBased.html#g-datagram-based-send-messagesAs MSG_DONTWAIT doesn't exist on Windows and g_datagram_based_send_messages cannot be used as the socket for TCP is not datagram type. changed the Api so that we set the socket to non blocking before sending and set it back again to blocking after sending.
Patch 358816, "Api to send messages to rtspconnection without blocking":
0001-Api-to-write-messages-to-rtspconnection-without-bloc.patchAttached the wrong patch file before
Patch 358934, "Api to send messages to rtspconnection without blocking":
0001-Api-to-write-messages-to-rtspconnection-without-bloc.patchIs this better than setting the socket to blocking before sending?
Patch 360351, "Api to send messages to rtspconnection without blocking":
0001-Api-to-write-messages-to-rtspconnection-alternate_patch.patchNew patch that fixes the use of eagain_err and only unblocks for the send call.
This patch it based on the same as the previous patch was based on.Patch 360948, "Api to write messages to rtspconnection without block":
0002-rtspconnection-Api-to-write-messages-to-rtspconnection-without-block.patch
@tpmsaid:As I already told Anila, I don't understand this whole nonblocking business here.
To me it seems like there are two separate concerns: a) something about non-blocking, not sure what that is ultimately supposed to fix, b) write bufferlists/vectors more efficiently.
The while loop in this patch looks to me like it does the exact same thing as a blocking send_messages call would do.
Hi Tim,
If we use the blocking send then we block in the send systemcall till the IPStack timeout is reached (which is around 120secs), thats the main reason we where trying to use non blocking send(for which we used the MSG_DONTWAIT flag in the beginning) because the thread comes out of the send systemcall as soon as we receive EWOULDBLOCK or EAGAIN.
Thanks & regards,
AnilaIt is true that gio is involved on master, where g_output_stream_write is called for example.
But here in gst_rtsp_connection_write_vectors the operations are done on the socket, and ultimately the system call sendmsg is called. The man page explains that the send() call can block if the message does not fit into the buffer.@tpmsaid:Again: is this based on you actually observing it blocking? Or is this based on how you think it will behave?
The man page says here.
When the message does not fit into the send buffer of
the socket, send() normally blocks, unless the socket has
been placed in nonblocking I/O mode. In nonblocking mode it
would fail with the error EAGAIN or EWOULDBLOCK in this case.My understanding is that gio always puts sockets into non-blocking mode, and that gst_socket_set_blocking() only affects whether gio functions will behave blockingly themselves (whilst always cancellable) or will return EWOULDBLOCK in case they would block.
Hi,
Sorry for the late response. I now have had time to test a bit.
I was not sure at first if the blocking issue was only based on assumption or not. However I found in the gio code, like you said, that it was setting the socket to non-blocking with g_unix_set_fd_nonblocking in g_socket_constructed.So I was under the assumption that it was non-blocking during the time I was testing. In the test case I am writing to a socket until it gets full. Then it hangs in g_socket_condition_timed_wait (in block_on_timeout, called from g_socket_send_message).
However, when I then explicitly set the socket to non-blocking before calling send_message, the call does not hang anymore but instead return an EWOULDBLOCK with a message "Error sending message: Resource temporarily unavailable".So this shows that the sockets created from GSocketConnection in the test cases are not put into non-blocking mode?
PS. sorry for the long buffer in the test case.
Patch 361405, "Test to show difference between block and non-block":
0007-rtspconnection-writing-block-and-non-block.patchI don't really understand the gsocket code just before it calls block_on_timeout though (https://git.gnome.org/browse/glib/tree/gio/gsocket.c#n4349).
timeout is != 0 if it is blocking (socket->priv->blocking ? -1 : 0).
So it checks if it is in blocking mode but at the same time it checks if errno is EWOULDBLOCK or EAGAIN. I thought those errors only appeared in non-blocking mode.But perhaps not, so it gets EWOULDBLOCK from sendmsg() and because socket->priv->blocking is TRUE it calls block_on_timeout, and that's where the test hangs unless the socket is explicitly set to non-blocking before the call.
(In reply to Tim-Philipp Müller from comment 10)
As I already told Anila, I don't understand this whole nonblocking business
here.To me it seems like there are two separate concerns: a) something about
non-blocking, not sure what that is ultimately supposed to fix, b) write
bufferlists/vectors more efficiently.The while loop in this patch looks to me like it does the exact same thing
as a blocking send_messages call would do.You are right, the actual socket is in non-blocking mode, and g_socket_set_blocking() decides whether gio will behave blocking or not. So there is no need to call g_socket_set_blocking().
Regarding the while loop, g_socket_send_message() behaves in a similar way. If sendmsg() returns an error it will try again. However, if sendmsg() manages to write a part of the data then g_socket_send_message() will not try to write the rest. It will simply return the number of bytes written. Thus the handling of partial writes in the while loop.
Unfortunately I discovered that while handling partial writes the new function changes the GOutputVectors that it gets as in-parameter. This may lead to weird behavior if the caller e.g. needs to de-allocate the data chunks.
One possible solution for this is to modify the function to reset vecs[i]->buffer and vecs[i]->size after it has handled a partial write. But I think that is kind of ugly, because it means that although the caller still has the ownership of the data, it is not allowed to use it while the call to the function is ongoing. Also the code in the while loop becomes slightly more complicated.
Another is to say that the function takes ownership of the GOutputVector array, and thus can do whatever it wants with it. The GOutputVecor array that is, not the actual data it points to. I think this is a clearer approach.
Which approach do you prefer, if any?
@tpmsaid:Regarding the while loop, g_socket_send_message() behaves in a similar way.
If sendmsg() returns an error it will try again. However, if sendmsg()
manages to write a part of the data then g_socket_send_message() will not
try to write the rest. It will simply return the number of bytes written.
Thus the handling of partial writes in the while loop.This is when the socket is in non-blocking mode, correct?
But when you set it to blocking mode, surely it will retry and loop until it has sent out all messages? (Or there's an error)
Unfortunately I discovered that while handling partial writes the new
function changes the GOutputVectors that it gets as in-parameter. This may
lead to weird behavior if the caller e.g. needs to de-allocate the data
chunks.Why is that? Not sure I understand the problem here. Could you explain?
One possible solution for this is to modify the function to reset
vecs[i]->buffer and vecs[i]->size after it has handled a partial write. But
I think that is kind of ugly, because it means that although the caller
still has the ownership of the data, it is not allowed to use it while the
call to the function is ongoing. Also the code in the while loop becomes
slightly more complicated.Another is to say that the function takes ownership of the GOutputVector
array, and thus can do whatever it wants with it. The GOutputVecor array
that is, not the actual data it points to. I think this is a clearer
approach.Which approach do you prefer, if any?
Not sure, I don't think it should take ownership of a GOutputVector? I would expect that a GOutputVector array is often/usually allocated by the caller on the stack anyway? Or in a caller-owned scratch area that is reused for subsequent calls.
(In reply to Tim-Philipp Müller from comment 18)
Regarding the while loop, g_socket_send_message() behaves in a similar way.
If sendmsg() returns an error it will try again. However, if sendmsg()
manages to write a part of the data then g_socket_send_message() will not
try to write the rest. It will simply return the number of bytes written.
Thus the handling of partial writes in the while loop.This is when the socket is in non-blocking mode, correct?
But when you set it to blocking mode, surely it will retry and loop until it
has sent out all messages? (Or there's an error)Nope, unless I misunderstand the code, as soon sendmsg() returns a value >= 0 g_socket_send_message() will return that value. Which I found a bit surprising. I had expected it to retry until all data was written, or a fatal error occured.
Unfortunately I discovered that while handling partial writes the new
function changes the GOutputVectors that it gets as in-parameter. This may
lead to weird behavior if the caller e.g. needs to de-allocate the data
chunks.Why is that? Not sure I understand the problem here. Could you explain?
The code here is from the function. vecs is the GOutputVector array.
... /* recalculate to deal with partial writes */ while (num_written > 0) { if (num_written < vecs[i].size) { vecs[i].buffer = (gchar *) vecs[i].buffer + num_written; vecs[i].size -= num_written; num_written = 0; } else { ...If sendmsg() failed to write all data of an GOutputVector we will end up in the if-statement. There the buffer-pointer and the size of that GOutputVector is modified, meaning that vecs[i].buffer no longer points to the same address that it pointed to when the call was made.
In my unit test the GOutputVector I pass to gst_rtsp_connection_write_vectors() points to malloc():ed data. After returning from the call the test deallocates the data, which gives funny errors when the pointers no longer points to the beginning of the malloc():ed data.
@tpmsaid:Sorry, I was thinking of send_messages() earlier (plural). My contention was that the entire while() loop, so most of the function, can be replaced with _send_messages() on a blocking socket instead.
I am not sure if the code you quote makes sense. I think a single message will always be fully written or not. It doesn't make sense to send a partial message/packet (and there's an error code for when it's too big). And makes even less sense to re-send parts of a message/packet, and there's no mechanism to say that this is the continuation from the previous one.
So I don't think we need to worry about this.
