After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 771830 - There is no time out in idle connection RTSP server
There is no time out in idle connection RTSP server
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-rtsp-server
git master
Other Linux
: Normal normal
: 1.11.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-09-22 12:44 UTC by Dag Gullberg
Modified: 2016-11-23 09:54 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch adding RTSP server timeout (3.67 KB, patch)
2016-09-22 12:49 UTC, Dag Gullberg
none Details | Review
Updated timout patch (3.67 KB, patch)
2016-09-22 14:58 UTC, Dag Gullberg
none Details | Review
Updated patch, timer attaching to watch context (4.01 KB, patch)
2016-11-21 09:58 UTC, Dag Gullberg
none Details | Review
Yet Another Updated Patch vol 1 (4.08 KB, patch)
2016-11-21 15:07 UTC, Dag Gullberg
none Details | Review
Yet Another Updated Patch vol 2 (4.11 KB, patch)
2016-11-22 09:07 UTC, Dag Gullberg
committed Details | Review

Description Dag Gullberg 2016-09-22 12:44:01 UTC
The RTSP server will not timeout an idle RTSP connection (note this is different from doing timeout on a RTSP session).

At least for Apache this is a problem when running RTSP over HTTPS since it uses one of the threads (there is a rather limited number) that are available for handling requests.

Actual behavior:
No timeout

Expected behavior:
Timeout (i.e.the RTSP server closes the connection if no command is received).

With e.g. telnet you can tests this.

1) Run “telnet 192.168.0.184 80”
2) Write the following HTTP request to start the RTSP connection
GET /axis-media/media.amp HTTP/1.0
Accept: application/x-rtsp-tunnelled
x-sessioncookie: 1212

3) Wait …
Comment 1 Dag Gullberg 2016-09-22 12:49:32 UTC
Created attachment 336081 [details] [review]
Patch adding RTSP server timeout

Does g_timeout_add of a callback simply counting up to a threshold where the client is closed when reached. The timer is removed upon SETUP  and also in client_watch_notify() called e.g if there is an attempt to do multiple request using same sessioncookie. Added safe guard is that for the client in question, before adding the times, any old one is removed.
Comment 2 Dag Gullberg 2016-09-22 14:58:42 UTC
Created attachment 336087 [details] [review]
Updated timout patch

Changed from g_timeout_add to g_timeout_add_seconds to minimize CPU usage, as the accuracy is not very important in IDLE.
Comment 3 Dag Gullberg 2016-09-29 12:14:51 UTC
Hi. Weekly ping. I know u guys are busy with release work, but it would be peachy if u could find time to review a couple of my patches. /Cheers
Comment 4 Dag Gullberg 2016-10-06 07:59:37 UTC
Just a remainder for review /Cheers
Comment 5 Dag Gullberg 2016-10-17 08:05:46 UTC
Please review
Comment 6 Matthew Waters (ystreet00) 2016-10-19 09:46:13 UTC
Review of attachment 336087 [details] [review]:

This seems ok.

::: gst/rtsp-server/rtsp-client.c
@@ +1835,3 @@
+  priv->rtsp_ctrl_timeout_cnt += RTSP_CTRL_CB_INTERVAL;
+
+  if (priv->rtsp_ctrl_timeout_cnt > RTSP_CTRL_TIMEOUT_VALUE)

Not entirely sure if this matters but the GSource will most likely not fire exactly every second due to system performance/rounding/etc.  If you want to accurately track time periods over a larger period of time, storing the current timestamp and comparing the current against the saved is a much better approach (IIRC, GSource has something to get the current wakeup time so that every GSource doesn't spam g_get_monotonic_time()).
Comment 7 Dag Gullberg 2016-10-19 09:52:38 UTC
There is no real need for accuracy here as there is no real-time content being propagated. It is just a rough timeout to stop the idle connection to infinitely hang.
Comment 8 Dag Gullberg 2016-10-26 08:26:05 UTC
Weekly remainder. Please review.
Comment 9 Dag Gullberg 2016-11-03 08:41:44 UTC
Please review
Comment 10 Dag Gullberg 2016-11-18 09:07:00 UTC
Please review this patch
Comment 11 Tim-Philipp Müller 2016-11-18 09:24:22 UTC
Comment on attachment 336087 [details] [review]
Updated timout patch

Looks mostly fine to me.

- please run gst-indent over the code, there are a few minor code style issues

>+  /* Setting up a timeout for the RTSP control channel until a session
>+   * is up where it is handling timeouts. */
>+  ...
>+  priv->rtsp_ctrl_timeout_id = g_timeout_add_seconds(RTSP_CTRL_CB_INTERVAL,
>+      rtsp_ctrl_timeout_cb, client);

If I'm not mistaken this adds the timeout to the default main context, but I don't think we can assume that the default main context will be iterated. Shouldn't we be using whatever main context the client was attached to? (priv->watch_context ?)
Comment 12 Nicolas Dufresne (ndufresne) 2016-11-18 19:10:01 UTC
I think so to, fyi, you should always use g_timeout_source_new_seconds() and attach the source to the right context in GStreamer.
Comment 13 Dag Gullberg 2016-11-21 09:58:50 UTC
Created attachment 340403 [details] [review]
Updated patch, timer attaching to watch context
Comment 14 Dag Gullberg 2016-11-21 10:00:44 UTC
ok, I have checked so I don't use main context but use the watch-context instead. I have tested it and it work fine. Also runned the c file through indent with appropriate parameters for code style
Comment 15 Tim-Philipp Müller 2016-11-21 11:31:36 UTC
Comment on attachment 340403 [details] [review]
Updated patch, timer attaching to watch context

Thanks for the updated patch.

I think you may have missed to git add the results after running gst-indent :)

Please also run 'make check' - quite a few tests fail now with criticals like:

   Source ID 4 was not found when attempting to remove it
Comment 16 Dag Gullberg 2016-11-21 12:39:47 UTC
:) Apologies, was a bit too quick. I confirm Source id4 situation. Will obsolete patch and return with a better one...
Comment 17 Dag Gullberg 2016-11-21 15:07:35 UTC
Created attachment 340428 [details] [review]
Yet Another Updated Patch vol 1
Comment 18 Dag Gullberg 2016-11-21 15:10:43 UTC
Have currently some issue with the build environment in the latest master, but did make check on a 1.9.2 setup, and it passed in the way the patch looks now.
Did a series of other tests as well on a axis camera platform. The code HAS now passed through indent.
Comment 19 Tim-Philipp Müller 2016-11-21 22:35:14 UTC
Comment on attachment 340428 [details] [review]
Yet Another Updated Patch vol 1

Thanks, but I get lots of leaks reported in 'make check-valgrind', e.g.

gst/rtspserver.valgrind

==2668== 544 (208 direct, 336 indirect) bytes in 2 blocks are definitely lost in loss record 5,594 of 5,684
==2668==    at 0x4C2CBC5: calloc (vg_replace_malloc.c:711)
==2668==    by 0x67CBE60: g_malloc0 (gmem.c:124)
==2668==    by 0x67C41FD: g_source_new (gmain.c:915)
==2668==    by 0x67C740E: g_timeout_source_new_seconds (gmain.c:4734)
==2668==    by 0x4E6AB0E: gst_rtsp_client_attach (rtsp-client.c:4346)
==2668==    by 0x4E6B28B: manage_client (rtsp-server.c:1057)
==2668==    by 0x4E6B5FC: gst_rtsp_server_io_func (rtsp-server.c:1191)
==2668==    by 0x59B7290: socket_source_dispatch (gsocket.c:3543)
==2668==    by 0x67C66A9: g_main_dispatch (gmain.c:3203)
==2668==    by 0x67C66A9: g_main_context_dispatch (gmain.c:3856)
==2668==    by 0x67C6A5F: g_main_context_iterate.isra.29 (gmain.c:3929)
==2668==    by 0x67C6B0B: g_main_context_iteration (gmain.c:3990)
==2668==    by 0x10C8E8: iterate (rtspserver.c:58)
==2668==    by 0x10D3EB: do_request_full (rtspserver.c:350)
==2668==    by 0x10F1D2: do_request (rtspserver.c:421)
==2668==    by 0x10F1D2: do_describe.constprop.4 (rtspserver.c:448)
==2668==    by 0x10F3E4: do_test_play_full.constprop.2 (rtspserver.c:1030)
==2668==    by 0x10FD6D: thread_func (rtspserver.c:1638)
==2668==    by 0x67EE344: g_thread_proxy (gthread.c:784)
==2668==    by 0x6A97463: start_thread (pthread_create.c:333)
==2668==    by 0x6D959DE: clone (clone.S:105)
Comment 20 Dag Gullberg 2016-11-22 09:06:09 UTC
Oppps... Shame on me... Forgot an unref.

New patch: Indent OK, Check OK. Valgrind OK
Comment 21 Dag Gullberg 2016-11-22 09:07:28 UTC
Created attachment 340493 [details] [review]
Yet Another Updated Patch vol 2
Comment 22 Tim-Philipp Müller 2016-11-23 09:53:44 UTC
Thanks, let's get this in then:

commit f00ac2daf244fecf59272bfd841685cc0af7d512
Author: Dag Gullberg <dagg@axis.com>
Date:   Mon Nov 21 16:02:39 2016 +0100

    rtsp-client: add IDLE timeout, before session exists
    
    The RTSP server will not timeout an idle RTSP connection
    (note this is different from doing timeout on a RTSP
    session).
    
    At least for Apache this is a problem when running RTSP over
    HTTPS since it uses one of the threads (there is a rather
    limited number) that are available for handling requests.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=771830