GNOME Bugzilla – Bug 167509
[PATCH] [tcpserversrc] Assumes a blocking socket
Last modified: 2005-02-25 08:49:15 UTC
Some code in tcpserversrc assumes a blocking socket, by expecting that read(socket, buffer, size) will either return an error, or will read exactly size bytes of data. As suggested by thomasvs on IRC, the solution is to wrap the read in a loop, until the required amount of data is fetched. A patch that does that is being tested as I write this.
Created attachment 37503 [details] [review] Patch fixing the problem
I'm not sure about the ret = read (...); if (ret <= 0) { error out } I think ret == 0 is a valid return value on a non-blocking socket, and signals that the other end has closed the socket. Will look into it in more detail tomorrow if I get around to it. Cheers -Tim
That "error out" returns ret, so the caller will know wether there is an error, or the remote end closed the connection. See gst_tcpserversrc_gdp_read_header(), which handles this case. (Note, the original code handled this case only in that function, too, just like after the patch)
What I meant is that you might be throwing away some data that you've read before the hang-up, but I see now that this is completely not an issue in this context; sorry for the noise. What about errno == EAGAIN? Should that really throw an error? Shouldn't we rather do a short sleep and continue the loop after that? Cheers -Tim
Sleeping does not sound good to me. I'd rather keep a temporary buffer around, and fill it with some data in each iteration, until it is completely filled, and then go on and process it.
Okay, there is one problem with my patch, I think.. Although I didn't test the behaviour without the patch (can't, as in that case the setup bails out with an error way too early). So, I have a flumotion-worker running on one box, which is using a simple flow composed of a producer pipeline (tcpserversrc protocol=1 host=10.42.0.1 ! oggmux) and a http-streamer component. Then, on another box, I do a videotestsrc ! theoraenc ! tcpclientsink protocol=1 host=10.42.0.1, and that works just fine until I stop the client. In that case, the flumotion-worker does not seem to notice that the client which fed it, disconnected. Reconnecting again does not fix the problem either. As far as I see, the problem is that when ret == 0, we should set the EOS state, and push out the appropriate event, but in gst_tcpserversrc_gdp_read_caps() we do neither. I'll try to fix that and submit an updated patch.
Same thing happens without my patch (just tried it with tcpserversrc and tcpclientsink being on the same host).
Hrm. I just tried with gst-launch tcpserversrc host=10.42.0.1 protocol=1 ! ffmpegcolorspace ! ximagesink on one box, gst-launch videotestsrc ! tcpclientsink protocol=1 host=10.42.0.1 on another, and that works. Also, looking into gst_tcpserversrc_get(), the element should return EOS when the peer disconnects.. So this might well be a bug in flumotion. I'll take further discussion to the appropriate forum.
The patch is sane. The byte loss is indeed there, but not a problem because we wouldn't be able to use them anyway (apart from data reading). So, applied, thanks.