GNOME Bugzilla – Bug 739799
rtspconnection: watch flushing doesn't flush output stream
Last modified: 2018-11-03 11:32:22 UTC
This is a tricky one. I'm not sure if this can be fixed in gstreamer or glib-networking. I have an RTSP server sending interleaved data in the RTSP TCP port. The problem is when TEARDOWN request is handled. The situation is like this: (1) rtsp connection sends (gst_rtsp_watch_write_data), let's day 1000 bytes. (2) this travels down to gnutls. (3) gnutls returns EAGAIN or EINTR. (4) rtsp connections records this data to be sent afterwards. (5) teardown request is handled. (6) watch flushing is TRUE (data saved in (4) is discarded). (7) watch flushing is FALSE. (8) TEARDOWN reply message is sent around 154 bytes. gnutls internally has buffered data from (1). it buffers it because the user is supposed to send exactly the same data again and gnutls has already encrypted the data, so it avoid re-encrypting it. However, this makes gnutls return the number of bytes from (1) instead of (8) which make rtsp connection go crazy and finally crashing. I think the right solution is to call g_output_stream_flush and implement the flush method in glib-networking. The flush would call: gnutls_record_send (gnutls->priv->session, NULL, 0); Which will flush the internal data. Another option is to check if the returned size from gnutls_record_send is greater than the one being sent. If so, call it again. Ot may be something in rtsp connection? This is the stack trace.
+ Trace 234318
Created attachment 290206 [details] [review] flush output stream I'm still verifying that this is the write fix. It bothers me a bit because gnutls will be sending buffered data that the application thought was never going to be sent. The counter argument of this is that the application is flushing so it doesn't really care wht was pending to be sent or not. How else could we avoid this?
Review of attachment 290206 [details] [review]: ::: gst-libs/gst/rtsp/gstrtspconnection.c @@ +3882,3 @@ + res = g_output_stream_flush (watch->conn->output_stream, + watch->conn->cancellable, &err); + Not really sure if we want the cancellable here as we really want it to block until done. WDYT?
For what it's worth, g_output_stream_flush() is a blocking call. The cancellable is most likely needed to avoid deadlock when doing to NULL state.
(In reply to comment #3) > For what it's worth, g_output_stream_flush() is a blocking call. The > cancellable is most likely needed to avoid deadlock when doing to NULL state. Oh, I see. I was wondering why it gets a cancellable if flush was blocking. That makes sense. The test seems to solve my crash. I added flush to glib-networking gnutls but I still need to test it a bit more. In any case, if this patch makes sense for GStreamer may be we can merge it? I'd like to clean it up a bit though first. WDYT?
I was thinking that flushing the output stream is not enough or correct. Shouldn't we send all the messages in rtspconnection queue instead of just emptying it? Otherwise we get an unpredictable behavior. The user has no idea which messages have been really sent but is probably assuming that all of them were sent. The solution from my patch in comment 1 only solve the specific gnutls problem where gnutls is buffering data. But still, the user will have no idea which messages have been sent. So, my feeling is that we should really send all remaining messages when flushing. WDYT?
(In reply to comment #5) > > So, my feeling is that we should really send all remaining messages when > flushing. > What I'm suggesting here is just the opposite of what gst_rtsp_watch_set_flushing documentation says.
Just filed bug 739983 with a patch implementing flush in glib-networking (gnutls).
Created attachment 290436 [details] [review] flush output stream clean Just added a debug message. This should be it.
As a side note. Patch for this bug and bug 739983 have been tested over ~300k connection/disconnection successfully.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/141.