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.

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.

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

Just a remainder for review /Cheers

Please review

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()).

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.

Weekly remainder. Please review.

Please review

Please review this patch

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 ?)

I think so to, fyi, you should always use g_timeout_source_new_seconds() and attach the source to the right context in GStreamer.

Created attachment 340403 [details] [review]
Updated patch, timer attaching to watch context

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 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

:) Apologies, was a bit too quick. I confirm Source id4 situation. Will obsolete patch and return with a better one...

Created attachment 340428 [details] [review]
Yet Another Updated Patch vol 1

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 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)

Oppps... Shame on me... Forgot an unref.

New patch: Indent OK, Check OK. Valgrind OK

Created attachment 340493 [details] [review]
Yet Another Updated Patch vol 2

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