Reference count janus_request instances

[lminiero]

While investigating #2005, and while @atoppi tried to replicate the issue, we found out that for some reason janus_request was one of the few structures still not using our reference counter mechanism. This janus_request structure contains a reference to the transport instance that originated it, and is actually used in two different ways:

1. any time we receive a request from a transport, we create an instance of janus_request and we destroy it when the request has been served (or acked, if it's an asynchronous plugin message);
2. when a session is created, we create an instance as well, in order to keep track of the transport instance we should send events to.

The latter in particular seems to be the root cause of issues like #2005: since this request object contains a reference to the transport instance, if that request is destroyed while it's used (e.g., in a send_message on the transport) it can cause a crash. The fact that this is not refcounted makes it of course much harder to track and protect accordingly.

As such, this patch adds refcount support to that struct, and adds a way to (try and) ensure it's not destroyed while it's used. Specifically, now each attempt to mess with session->source (which is the pointer the janus_request the session owns) is protected by the session mutex: rather than just wrap any attempt to use that request and its transport (e.g., to interact with transport plugins), I added a new method that, using the mutex, copies the pointer and increases the refcount, while we work on the copy for what we need to do. Basically, you'll find something like this:

janus_request *source = janus_session_get_request(session);
if(source && source->transport)
	source->transport->session_over(source->instance, session->session_id, FALSE, FALSE);
janus_request_unref(source);

where janus_session_get_request returns a pointer to session->source with an additional reference (hence the janus_request_unref after we've used it) which has been obtained locking the session mutex. This way, even if something else unrefs the session request, it will not be destroyed until we're actually done with it.

This should also mitigate some risk of deadlock we had before, where we were invoking transport methods with the session mutex locked: while none of the transports invoked a core callback from those method, thus risking a new session mutex lock, new or third party transports might, and so this now avoids it, as we only use the mutex to get a reference to the request we need, before invoking the transport method.

I only did some brief tests here, and it doesn't seem to introduce leaks, but of course we'll need some more extensive testing to be sure this does indeed fix the HTTP issue mentioned in #2005, and if it does that it doesn't introduce regressions in HTTP or other transports.

[atoppi]

After latest changes I'm not able to crash it anymore, so lgtm 👍

[agclark27]

We're running Lorenzo's commits here in our production environments as of about 12 hours ago, and we're seeing very positive results as well. I'll be contributing a separate PR for review in a short bit that includes some other changes we've made that are improving stability.

[lminiero]

@agclark81 you may want to start a fresh test with the latest commits, as we've fixed a few other (unrelated to the original issue) crashes that appeared. But if you can confirm the original crash isn't there anymore, that's good news indeed!

[lminiero]

@agclark81 any objections to merge this? I'd like to tag a new release soon (possibly by today), and this is one of the fixes I want to be included since it's the release providing the HTTP refactoring.

[agclark27]

Seems OK to merge, and we've been testing with the additional HTTP refactoring, too, and everything is working well.

[lminiero]

Thanks, merging then! Looking forward to the additional fixes you mentioned 👍