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 749567 - tcpclientsrc: add timeout property
tcpclientsrc: add timeout property
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal enhancement
: 1.11.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-05-19 08:01 UTC by Nicola
Modified: 2016-12-21 08:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
proposed patch (3.11 KB, patch)
2015-05-19 08:01 UTC, Nicola
none Details | Review
updated patch (3.54 KB, patch)
2015-06-04 12:14 UTC, Nicola
none Details | Review
updated patch (3.55 KB, patch)
2015-06-04 12:15 UTC, Nicola
none Details | Review
updated patch (3.55 KB, patch)
2015-06-04 12:16 UTC, Nicola
none Details | Review
updated patch (3.65 KB, patch)
2016-12-20 11:36 UTC, Nicola
committed Details | Review

Description Nicola 2015-05-19 08:01:27 UTC
Created attachment 303571 [details] [review]
proposed patch

actually tcpclientsrc will never timeout, this can lead to weired bug in non optimal network conditions. Added a property that default to 0 (no timeout)
Comment 1 Prashant Gotarne 2015-06-01 11:29:22 UTC
Review of attachment 303571 [details] [review]:

I think its better to set the max value of timeout property to G_MAXUINT instead of 3600. 
Let's give freedom to user to choose any value from 0 to G_MAXUINT.

Also adding use case to explain the need of timeout property.
Run a server on one machine.
#server-107.108.234.23:
nc -l -p 3000

Run following pipeline on another machine
#client-107.108.234.56:
gst-launch tcpclientsrc host="107.108.234.23" port=3000 ! fdsink fd=2

If we pull the network cable from the client machine, it never detects 
the network failure and keeps waiting for the data from server.

Setting timeout value can be useful here.
Comment 2 Nicola 2015-06-01 11:46:20 UTC
(In reply to Prashant Gotarne from comment #1)
> Review of attachment 303571 [details] [review] [review]:

thanks for the review!

> 
> I think its better to set the max value of timeout property to G_MAXUINT
> instead of 3600. 

I used 3600 as souphttpsrc (I thought was an accepted value), no problem to change to G_MAXUINT even if I don't see any pratical use case for a so high timeout (tcp stack will however timeout after 7200 seconds if you don't change kernel settings)

> Let's give freedom to user to choose any value from 0 to G_MAXUINT.
> 
> Also adding use case to explain the need of timeout property.
> Run a server on one machine.
> #server-107.108.234.23:
> nc -l -p 3000
> 
> Run following pipeline on another machine
> #client-107.108.234.56:
> gst-launch tcpclientsrc host="107.108.234.23" port=3000 ! fdsink fd=2
> 
> If we pull the network cable from the client machine, it never detects 
> the network failure and keeps waiting for the data from server.
> 
> Setting timeout value can be useful here.

ok, I'll add that, 

probably the patch need a "Since: 1.6" too if you plan to push before 1.6 release, can you please confirm?
Comment 3 Nicola 2015-06-04 12:14:30 UTC
Created attachment 304576 [details] [review]
updated patch
Comment 4 Nicola 2015-06-04 12:15:27 UTC
Created attachment 304577 [details] [review]
updated patch
Comment 5 Nicola 2015-06-04 12:16:36 UTC
Created attachment 304578 [details] [review]
updated patch
Comment 6 Nicola 2015-07-09 10:29:53 UTC
any updates? would be useful to include this patch in 1.6 a tcpclient with no timeout will not work in many real world use cases
Comment 7 Nicola 2016-02-15 12:24:48 UTC
ping
Comment 8 Nicola 2016-12-19 16:22:58 UTC
what is missing to get this patch merged?
Comment 9 Sebastian Dröge (slomo) 2016-12-20 10:40:44 UTC
Review of attachment 304578 [details] [review]:

Generally looks good, just some minor things

::: gst/tcp/gsttcpclientsrc.c
@@ +33,3 @@
  * gst-launch-1.0 tcpclientsrc port=3000 ! fdsink fd=2
+ * ]| everything you type in the server is shown on the client.
+ * If you want to detect network failures and/or limit the time your tcp client keeps waiting for data from server setting a timeout value can be useful.

You probably want to split this into multiple lines ;)

@@ +115,3 @@
+      g_param_spec_uint ("timeout", "timeout",
+          "Value in seconds to timeout a blocking I/O. 0 = No timeout. "
+          "(Since 1.6)", 0, G_MAXUINT, TCP_DEFAULT_TIMEOUT,

Since: 1.12, and this shouldn't be in the property description but as gtk-doc documentation for the property

@@ +408,3 @@
     goto no_socket;
 
+  g_socket_set_timeout (src->socket, src->timeout);

Now socket operations will timeout. Should we handle the timeout error return value specifically somehow?
Comment 10 Nicola 2016-12-20 11:36:15 UTC
Created attachment 342256 [details] [review]
updated patch

timeout error seems already properly handled, please take a look at the below pipeline

time gst-launch-1.0 -vmt tcpclientsrc host=192.168.2.123 port=123 timeout=10 ! fakesink
Setting pipeline to PAUSED ...
ERROR: Pipeline doesn't want to pause.
Got message #3 from element "fakesink0" (state-changed): GstMessageStateChanged, old-state=(GstState)GST_STATE_NULL, new-state=(GstState)GST_STATE_READY, pending-state=(GstState)GST_STATE_VOID_PENDING;
Got message #4 from element "tcpclientsrc0" (state-changed): GstMessageStateChanged, old-state=(GstState)GST_STATE_NULL, new-state=(GstState)GST_STATE_READY, pending-state=(GstState)GST_STATE_VOID_PENDING;
Got message #5 from element "pipeline0" (state-changed): GstMessageStateChanged, old-state=(GstState)GST_STATE_NULL, new-state=(GstState)GST_STATE_READY, pending-state=(GstState)GST_STATE_PAUSED;
Got message #8 from element "tcpclientsrc0" (error): GstMessageError, gerror=(GError)NULL, debug=(string)"gsttcpclientsrc.c\(458\):\ gst_tcp_client_src_start\ \(\):\ /GstPipeline:pipeline0/GstTCPClientSrc:tcpclientsrc0:\012Failed\ to\ connect\ to\ host\ \'192.168.2.123:123\':\ Socket\ I/O\ timed\ out";
ERROR: from element /GstPipeline:pipeline0/GstTCPClientSrc:tcpclientsrc0: Could not open resource for reading.
Additional debug info:
gsttcpclientsrc.c(458): gst_tcp_client_src_start (): /GstPipeline:pipeline0/GstTCPClientSrc:tcpclientsrc0:
Failed to connect to host '192.168.2.123:123': Socket I/O timed out
Setting pipeline to NULL ...
Freeing pipeline ...

real	0m10,058s
user	0m0,020s
sys	0m0,023s

setting timeout=0 make the pipeline waits for ever
Comment 11 Sebastian Dröge (slomo) 2016-12-21 08:06:06 UTC
commit 07646dd11b64e3dc4296b8b2f1ab55e5b881d83b
Author: Nicola Murino <nicola.murino@gmail.com>
Date:   Tue Dec 20 12:33:12 2016 +0100

    tcpclientsrc: add timeout property
    
    https://bugzilla.gnome.org/show_bug.cgi?id=749567