GNOME Bugzilla – Bug 749567
tcpclientsrc: add timeout property
Last modified: 2016-12-21 08:06:51 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)
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.
(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?
Created attachment 304576 [details] [review] updated patch
Created attachment 304577 [details] [review] updated patch
Created attachment 304578 [details] [review] updated patch
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
ping
what is missing to get this patch merged?
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?
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
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