GNOME Bugzilla – Bug 756000
gstrtmpsink: add unlock
Last modified: 2016-09-06 17:58:38 UTC
Created attachment 312590 [details] [review] patch Same principle as in gstrtmpsrc, the idea is that librtmp might block on a tcp-send, causing a deadlock when trying to shut down rtmpsink. By implementing unlock, the socket will be closed and the ongoing send will be terminated. (depending on the version of librtmp used)
I think an approach like in bug #755732 would make sense here too instead of this. What do you think?
Review of attachment 312590 [details] [review]: ::: ext/rtmp/gstrtmpsink.c @@ +197,3 @@ + * error out. */ + if (rtmpsink->rtmp) { + RTMP_Close (rtmpsink->rtmp); unlock() is not only called on shutdown IIRC. Is anything still usable afterwards? Should we do something in unlock_stop() to make things usable again?
Also, is this thread-safe in librtmp?
Actually, this patch needs updating, as this will crash occasionally, with the rtmp-pointer being freed in another thread (and you can't lock since that defies what unlock is all about) What you need to do (unfortunately) is to call shutdown on the internal RTMP socket, to cancel the (potentially) blocking send/recv. I will update the patch and try and supply a test or two as well.
Created attachment 324976 [details] [review] updated patch with tests The main problem this patch is trying to solve, is that librtmp can poentially block a connection or a write for a very long time, potentially stalling the application for as long as a TCP timeout can last (30 sec) However, doing this in a MT safe way is much harder then it would appear. The "most common" scenario would be that librtmp is currently doing a write on the socket, and we terminate this write by using shutdown on that internal socket. Now, had we instead used RTMP_Close, we will often interfer with internal state inside librtmp as they are being used, so shutdown is our only option here. We need to add a close() as well, since the tests showed that even though we successfully cancelled one write, while doing a handshake there can be another one as well, and only by doing close are we preventing that second one from happening. By also protecting the internal state with a lock, we then use _trylock to figure out that we in fact are not allowed to touch the internal state, but then instead use shutdown... Finally, a looping wait is added for the case where the rtmp-lock has been taken, but the internals of librtmp have not yet allocated the socket it is about to work with, meaning our shutdown would fail silently, and we would still be waiting "forever". All of these changes are needed to make the supplied tests pass. Maybe an atomic counter could be more elegant then using lock and trylock, but it felt like the right solution, since we are both eliminating the internal state completely, and when not allowed to do so, using shutdown to break the wait.
Review of attachment 324976 [details] [review]: ::: ext/rtmp/gstrtmpsink.c @@ +227,3 @@ + the socket comes up */ + while (sink->connecting && sink->rtmp->m_sb.sb_socket == -1) { + g_thread_yield (); Could be slightly cleaner and more efficient using a GCond here. ::: ext/rtmp/gstrtmpsink.h @@ +53,3 @@ RTMP *rtmp; gchar *rtmp_uri; /* copy of url for librtmp */ + GMutex rtmp_lock; I believe with some effort, using the stream lock should be possible. This way, you would have unlock() that always do shutdown, and unlock_stop() (which have the state lock held) that always do RTMP_Close(). Any other blocking operation should be done where the stream lock is held already, if not, that's a bug.
(In reply to Nicolas Dufresne (stormer) from comment #6) > Review of attachment 324976 [details] [review] [review]: > > ::: ext/rtmp/gstrtmpsink.c > @@ +227,3 @@ > + the socket comes up */ > + while (sink->connecting && sink->rtmp->m_sb.sb_socket == -1) { > + g_thread_yield (); > > Could be slightly cleaner and more efficient using a GCond here. How do you envision that? We are waiting for an internal socket inside librtmp to become initialized, and there is no signaling from librtmp to say this has happened. If you have a clever trick I am very interested, as I don't like this either, but it was the only thing I could think of to make the tests pass. > > ::: ext/rtmp/gstrtmpsink.h > @@ +53,3 @@ > RTMP *rtmp; > gchar *rtmp_uri; /* copy of url for librtmp */ > + GMutex rtmp_lock; > > I believe with some effort, using the stream lock should be possible. This > way, you would have unlock() that always do shutdown, and unlock_stop() > (which have the state lock held) that always do RTMP_Close(). Any other > blocking operation should be done where the stream lock is held already, if > not, that's a bug. I have never been a fan of touching the stream-lock in implementations, as in my experience this is a certain path to deadlocks, and I have always considered it an internal lock. I'd much rather have an additional "small" well-defined, easily scoped lock then piggy-backing on one that might have unforeseen and complex interactions.
Do we ship librtmp on other OS? If so, does this work on windows? Another issue is more conceptual: it seems to me that the assumption here is that unlock=shutdown, but theoretically it could have been triggered by a flushing seek for example - would one want to re-connect in that case?
(In reply to Tim-Philipp Müller from comment #8) > Do we ship librtmp on other OS? Yes
(In reply to Tim-Philipp Müller from comment #8) > Another issue is more conceptual: it seems to me that the assumption here is > that unlock=shutdown, but theoretically it could have been triggered by a > flushing seek for example - would one want to re-connect in that case? That could simply be fixed by reconnecting in unlock_stop() I believe. There is no way to unlock network operations without having to reset the connection. That is true for many protocols.
(In reply to Håvard Graff (hgr) from comment #7) > (In reply to Nicolas Dufresne (stormer) from comment #6) > > Review of attachment 324976 [details] [review] [review] [review]: > > > > ::: ext/rtmp/gstrtmpsink.c > > @@ +227,3 @@ > > + the socket comes up */ > > + while (sink->connecting && sink->rtmp->m_sb.sb_socket == -1) { > > + g_thread_yield (); > > > > Could be slightly cleaner and more efficient using a GCond here. > > How do you envision that? We are waiting for an internal socket inside > librtmp to become initialized, and there is no signaling from librtmp to say > this has happened. If you have a clever trick I am very interested, as I > don't like this either, but it was the only thing I could think of to make > the tests pass. True, you could signal sink->connecting state, but probably not rtmp->mb_sb.sb_socket. > > > > > ::: ext/rtmp/gstrtmpsink.h > > @@ +53,3 @@ > > RTMP *rtmp; > > gchar *rtmp_uri; /* copy of url for librtmp */ > > + GMutex rtmp_lock; > > > > I believe with some effort, using the stream lock should be possible. This > > way, you would have unlock() that always do shutdown, and unlock_stop() > > (which have the state lock held) that always do RTMP_Close(). Any other > > blocking operation should be done where the stream lock is held already, if > > not, that's a bug. > > I have never been a fan of touching the stream-lock in implementations, as > in my experience this is a certain path to deadlocks, and I have always > considered it an internal lock. I'd much rather have an additional "small" > well-defined, easily scoped lock then piggy-backing on one that might have > unforeseen and complex interactions. You don't take the streaming lock, that's not the element job. But all operation that may need to be cancelled, should happen under this lock only. When you start adding extra lock, and doing blocking operation from non-streaming threads, that's where you start having issues. If all the operation on the RTMP library where done within the stream thread, they could be protected by the stream lock, and you would not need to add new lock. The only remaining call, that will happen from external thread is start()/stop(), but the streaming thread should not be running at this point. And then you also have to deal with _unlock(), which minially unblock the operations, while unlock_stop() shall do the cleanup as it's called with the stream lock.
> > Another issue is more conceptual: it seems to me that the assumption here is > > that unlock=shutdown, but theoretically it could have been triggered by a > > flushing seek for example - would one want to re-connect in that case? > > That could simply be fixed by reconnecting in unlock_stop() I believe. But then it's not a continuous stream any more, which is what my question boils down to really. unlock_stop() isn't really supposed to interfere with that (but I could live with this in this context). > There is no way to unlock network operations without having to > reset the connection. That is true for many protocols. I don't entirely understand this comment, could you elaborate what you mean? Are you talking about the TCP protocol level or some other protocol? Isn't this mostly an implementation detail? The rtmp implementation could surely be done in a way that's always fully async and fully interruptible (worst case you leave behind a thread that's stuck in some non-async connect or dns lookup, no?)?
(In reply to Tim-Philipp Müller from comment #12) > But then it's not a continuous stream any more, which is what my question > boils down to really. unlock_stop() isn't really supposed to interfere with > that (but I could live with this in this context). Unlocking a source/sink cause some lost of data in general. Not sure it matters really. > > > There is no way to unlock network operations without having to > > reset the connection. That is true for many protocols. > > I don't entirely understand this comment, could you elaborate what you mean? > Are you talking about the TCP protocol level or some other protocol? Isn't > this mostly an implementation detail? The rtmp implementation could surely > be done in a way that's always fully async and fully interruptible (worst > case you leave behind a thread that's stuck in some non-async connect or dns > lookup, no?)? That could be another way to support librtmp indeed. Spawn a thread, and do everything in that single thread. That thread would ensure client/server state stay sane, whatever happens. We know the librtmp is not thread-safe, we should probably stop hacking around.
(In reply to Nicolas Dufresne (stormer) from comment #13) > (In reply to Tim-Philipp Müller from comment #12) > > I don't entirely understand this comment, could you elaborate what you mean? > > Are you talking about the TCP protocol level or some other protocol? Isn't > > this mostly an implementation detail? The rtmp implementation could surely > > be done in a way that's always fully async and fully interruptible (worst > > case you leave behind a thread that's stuck in some non-async connect or dns > > lookup, no?)? > > That could be another way to support librtmp indeed. Spawn a thread, and do > everything in that single thread. That thread would ensure client/server > state stay sane, whatever happens. We know the librtmp is not thread-safe, > we should probably stop hacking around. Except you will still be left with the problem I clearly demonstrate in the supplied tests, and this patch solves: unblocking an ongoing connect or write. Also, what do you mean by librtmp not being thread-safe? We have had conferences with 50+ people connected using rtmp, and have not had any issues. What issues have you had with it?
The library, when I last looked had no locking, hence should not be called from concurrent threads.
(In reply to Nicolas Dufresne (stormer) from comment #15) > The library, when I last looked had no locking, hence should not be called > from concurrent threads. It does not have any locking no, which is why the patch uses the TRY_LOCK trick, and does a shutdown on the internal socket if the lock is in use. (which is what rtmpsrc did back in 0.10)
The question we are trying to get answer before merging is: What happens to the RTMP connection after you have shutdown the socket behind it's back. Can you flush a pipeline with rtmpsink without getting an error ?
I don't know. AFAICT these are the first tests for rtmpsink, so there are a lot of unknowns I guess. After calling _unlock(), the _start() method will need to be called again in order for it to be able to work again, since currently unlock will tear down the RTMP-object. Maybe attaching to the state-changes is a better idea? _unlock() is called from gstbasesink.c when going from PLAYING to PAUSED, so maybe implementing state-changes and then doing the "unlocking" in that state change is a better idea, if the _unlock() concept is a bit overloaded?
I don't think the concept is overloaded, but perhaps your implementation is a bit "underloaded" :) (sorry, not helpful, I know)
We have plans for new rtmp elements not using librtmp at all, so I won't bother pushing more patches for these. Our version of these elements can be found here: https://github.com/pexip/gst-plugins-bad/tree/master/ext/rtmp
(In reply to Håvard Graff (hgr) from comment #20) > We have plans for new rtmp elements not using librtmp at all, so I won't > bother pushing more patches for these. Our version of these elements can be > found here: https://github.com/pexip/gst-plugins-bad/tree/master/ext/rtmp Is this the right link? Looks like the old ones to me? Are yours based on the gio-based rewrites from David Schleef (https://cgit.freedesktop.org/~ds/gst-rtmp/), or did you roll your own?
(In reply to Tim-Philipp Müller from comment #21) > (In reply to Håvard Graff (hgr) from comment #20) > > We have plans for new rtmp elements not using librtmp at all, so I won't > > bother pushing more patches for these. Our version of these elements can be > > found here: https://github.com/pexip/gst-plugins-bad/tree/master/ext/rtmp > > > Is this the right link? Looks like the old ones to me? > > Are yours based on the gio-based rewrites from David Schleef > (https://cgit.freedesktop.org/~ds/gst-rtmp/), or did you roll your own? sorry, badly explained. The link are to the elements we are using now. The new ones will bundle https://github.com/pexip/pexrtmpserver and are still under planning. :)
Let us know when that's ready, we will be really pleased to replace the existing element with something thread safe.