GNOME Bugzilla – Bug 767501
curlsmtpsink: Lock and don't send final boundary upon error
Last modified: 2016-06-11 10:26:45 UTC
Previously GstCurlSmtpSink could cause the pipeline thread to end up waiting for a stopped thread to perform work. The scenario was that the sink could be rendering a buffer and waiting for the curl transfer thread to have sent the data. As soon as the transfer thread has copied all data to curl's data buffer in gst_curl_base_sink_transfer_read_cb() then the render call would stop waiting and return GST_FLOW_OK. While this takes place the transfer thread may suffer from an error e.g. due gst_poll_wait() timing out. This causes the transfer thread to record the error, claim (it is not really true since there was an error) that the data has been sent and that a response has been received by trying to signal the pipeline thread (but this has already stopped waiting). Finally the transfer thread stops itself. A short while later the pipeline thread may attempt to push an EOS event into GstCurlSmtpSink. Since there is no check in gst_curl_smtp_sink_event() to check if the sink has suffered from any error it may attempt to add a final boundary and ask the, now deceased, transfer thread to transfer the new data. Next the sink element would have waited for the transfer to complete (using a different mechanism than normal transfers through GstCurlBaseSink). In this case there was an error check to avoid waiting if an error had already been seen. Finally GstCurlSmtpSink would chain up to GstCurlBaseSink which would then block waiting for a response (normally this would be prevented by the transfer thread suffering the error claiming that it had been received, but GstCurlSmtpSink clobbered this flag after the fact). Now GstCurlSmtpSink avoids this by locking over the entire event handing (preventing simultaneous changes to flags by the two threads) and also by avoiding to initiate transfer of final boundary if an error has already been seen. Also add GST_FIXME() for remaining similar issue where the pipeline thread may block indefinitely waiting for transfer thread to transfer data but the transfer thread errors out and fails to notify the pipeline thread that the transfer failed.
Created attachment 329580 [details] [review] Proposed patch.
Pushed, thanks (after fixing up indentation, please install gnu indent): commit c7e421712155cc01b42bee9adea253134ceea5c1 Author: Sebastian Rasmussen <sebrn@axis.com> Date: Thu Jun 9 01:20:36 2016 +0200 curlsmtpsink: Lock and don't send final boundary upon error Previously GstCurlSmtpSink could cause the pipeline thread to end up waiting for a stopped thread to perform work. The scenario was that the sink could be rendering a buffer and waiting for the curl transfer thread to have sent the data. As soon as the transfer thread has copied all data to curl's data buffer in gst_curl_base_sink_transfer_read_cb() then the render call would stop waiting and return GST_FLOW_OK. While this takes place the transfer thread may suffer from an error e.g. due gst_poll_wait() timing out. This causes the transfer thread to record the error, claim (it is not really true since there was an error) that the data has been sent and that a response has been received by trying to signal the pipeline thread (but this has already stopped waiting). Finally the transfer thread stops itself. A short while later the pipeline thread may attempt to push an EOS event into GstCurlSmtpSink. Since there is no check in gst_curl_smtp_sink_event() to check if the sink has suffered from any error it may attempt to add a final boundary and ask the, now deceased, transfer thread to transfer the new data. Next the sink element would have waited for the transfer to complete (using a different mechanism than normal transfers through GstCurlBaseSink). In this case there was an error check to avoid waiting if an error had already been seen. Finally GstCurlSmtpSink would chain up to GstCurlBaseSink which would then block waiting for a response (normally this would be prevented by the transfer thread suffering the error claiming that it had been received, but GstCurlSmtpSink clobbered this flag after the fact). Now GstCurlSmtpSink avoids this by locking over the entire event handing (preventing simultaneous changes to flags by the two threads) and also by avoiding to initiate transfer of final boundary if an error has already been seen. Also add GST_FIXME() for remaining similar issue where the pipeline thread may block indefinitely waiting for transfer thread to transfer data but the transfer thread errors out and fails to notify the pipeline thread that the transfer failed. https://bugzilla.gnome.org/show_bug.cgi?id=767501