Added support for admin-protected custom session timeouts #2577

alg · 2021-03-09T12:47:03Z

Adds support for specifying custom session timeouts during session creation. Custom timeouts are protected behind admin_secret and not allowed for general purpose scenarios. Default timeout is the one defined globally (currently 60 seconds).

This is useful for backend-initiated sessions that can't be regularly pinged. At the same time all other client-initiated sessions should respect the global setting.

Example session with 1 hour timeout:

{
    "janus": "create",
    "transaction": "trx-create",
    "admin_secret": "janusoverlord",
    "timeout": 3600
}

januscla · 2021-03-09T12:47:05Z

Thanks for your contribution, @alg! Please make sure you sign our CLA, as it's a required step before we can merge this.

lminiero · 2021-03-12T11:32:40Z

Thanks for the contribution, but I don't like the idea of mixing Janus and Admin API concepts, in this case the usage of the admin password from the Janus API. The "create" should remain as is, IMO, with possibly a new Admin API method to dynamically change the timeout value for a specific session. This would allow a controlling application to do both calls when needed, and also change the timeout value later rather than at startup (in case it's ever needed).

Currently busy with the IETF meeting so I can't review the code in detail, will try to do this next week 👍

alg · 2021-03-12T17:50:33Z

@lminiero Sounds equally good to me. I didn't see anything bad with setting the session value there since it's the place it was set to default global anyway and the admin password was just the protection not to let anyone choose the value freely. If you feel a separate call would be a better shot, I can rework it. Maybe give it another thought while you are away. Let me know.

lminiero

I still think it should be a separate request, and only in the Admin API, on existing sessions created the "old" way: it's cleaner in terms of separation of concerns for the APIs, and as anticipated allows for dynamic tweaking of the timeout value also after the fact.

For the rest, I only added a small note on code style for comments. The code changes look fine to me, with the difference of course that now create should ensure the new timeout property is initialized to the global one: any change to that can be enforced via whatever new request will be added to the Admin API.

lminiero · 2021-03-16T11:03:02Z

janus.c

@@ -1063,8 +1062,30 @@ int janus_process_incoming_request(janus_request *request) {
 				goto jsondone;
 			}
 		}
+
+		// Parse session timeout


Please use /* */ for comments (code style).

alg · 2021-03-16T13:30:08Z

Ok, got it. Will update the PR shortly.

alg · 2021-03-16T15:34:41Z

@lminiero Just pushed the updated version. See if you like it.

lminiero · 2021-03-22T10:17:39Z

Apologies for the late reply @alg. It looks like this is still done as part of the Janus API, though, not the Admin API: the only change I see is that it's done in a separate request and not at session creation time anymore, but it's still the Janus API. As per the exchanges we had, this should be an Admin API method.

alg · 2021-03-22T13:53:56Z

@lminiero Sure, no problem. I think I misunderstood your previous comment. Apologies for that. I noticed there's already set_session_timeout in admin section that currently sets the global session timeout value. So I created a similar call in session-specific section of admin API and directed both to the same handler that deals with both cases. See if you like it.

lminiero

Thanks for the quick fixes! I added some inline notes: to summarize, I think we don't need the helper functions, as the action is simple enough to be done inline (which would make the new request fit more with the others as well).

lminiero · 2021-03-22T15:32:44Z

janus.c

-		if(strcasecmp(message_text, "list_handles")) {
+		} else if (!strcasecmp(message_text, "set_session_timeout")) {
+			/* Specific session timeout setting. */
+			ret = janus_process_set_session_timeout(request, session, transaction_text, root);


See above: the action seems relatively simples that it could be done inline, as the other requests handled here.

lminiero · 2021-03-22T15:33:57Z

janus.h

+/*! \brief Method to set session timeout value.
+ * @param[in] session The Janus Core-Client session
+ * @param[in] timeout Timeout value in seconds. */
+void janus_session_set_timeout(janus_session *session, uint timeout);


We don't really need the function here, I think. It's probably my fault as initially I put a lot of functions in this header that really didn't need exposing, as they're only used within janus.c anyway.

lminiero · 2021-03-22T15:35:05Z

janus.h

+ * @param[in] root The request payload root
+ * @returns 0 on success, a negative integer otherwise
+ */
+int janus_process_set_session_timeout(janus_request *request, janus_session *session, const gchar *transaction, json_t *root);


See above. I actually think that, besides removing them from the header, we can get rid of both methods and do it more simply inline where the requests are processed, since the update functionality is simple.

lminiero · 2021-03-22T15:37:02Z

janus.c

-			json_object_set_new(reply, "timeout", json_integer(session_timeout));
-			/* Send the success reply */
-			ret = janus_process_success(request, reply);
+			ret = janus_process_set_session_timeout(request, session, transaction_text, root);


Whoops, looks like this comment didn't make it through with the others. I basically wrote that I don't understand why this needed to change, and that it feels confusing to use the same method for two different things.

alg · 2021-03-23T08:02:33Z

@lminiero Updated.

To tell the truth, I'm a bit overwhelmed by the source code. A file with 5.5k lines is something I've never seen before in my 30 years of software development practice. It's your project and you decide hot to evolve it, but it's already hard to navigate and understand with lots of duplication and all these goto's. I attempted to extract handling of set session timeout action to keep it in one place. Not sure why you told they do radically different things. They both change session timeout (global or specific session), do the same data parsing / checks and return almost the same set of fields. Again, your project, your vision.

lminiero · 2021-03-23T14:11:24Z

I get your point, but right now that's how the code is structured, and it needs to be consistent. While we may change this in the future, it needs to be done for everything and not just parts of it, otherwise it only adds to the confusion.

Thanks for the changes, I'll make some tests. I just realized this may be a breaking change for some, as before when changing the global session timeout this had an impact on all sessions: now this will only have an impact on new sessions created after the change, because existing sessions (even those that still have the default value) will be stuck with the old one, and would need to be updated independently. It may be worthwhile to add a new setting to also update the timeout value of some or all sessions when changing the global one (e.g., "if session->timeout == old timeout, update that one too), but that's something we can do in another PR later on. This breaking change means we need to merge only when we tag a major version change, though: the next one will be 0.11.0 so it should be ok.

alg · 2021-03-24T07:02:30Z

I wonder if it's a wanted contribution at all. If not, I'm fine with it. I can always fork. No need to go through this lengthy routine.

If it is, then instead of writing a global session timeout into a specific session, we can write -1 showing that there's no local session override and a global timeout value should be used. If there's a non-negative value in the session, it should be used instead.

lminiero · 2021-03-24T07:57:59Z

I wonder if it's a wanted contribution at all. If not, I'm fine with it. I can always fork. No need to go through this lengthy routine.

I'm sorry you got this from our exchanges, it was not my intention. Of course I do appreciate the contribution, and see value in it. At the same time, as the main maintainer of the project I have to always be extremely careful of the impact contributions can have, as once they're in they're my responsibility: Janus is used in a ton of deployments, and what can be considered breaking changes needs to be taken into account. If you think I was asking you to worry about this, I wasn't: I explicitly said in my previous post it's something we would have taken care of ourselves later on.

If it is, then instead of writing a global session timeout into a specific session, we can write -1 showing that there's no local session override and a global timeout value should be used. If there's a non-negative value in the session, it should be used instead.

This might be a good solition, since the "scope" of a session timeout is small enough that we can use a signed integer for it (most of our values are uint64_t so that wouldn't be possible otherwise).

alg · 2021-03-25T07:28:46Z

Good to know it's something the project would benefit from.

I changed it as discussed above. See if you like it.

lminiero · 2021-03-25T11:26:54Z

Thanks, from a quick glance it looks good, I'll test it later!
The only thing I noticed is there's still a leftover janus_session_set_timeout definition in janus.h.

alg · 2021-03-25T12:45:53Z

You are right. Sorry, missed that bit.

lminiero

There were some warnings during the compilation, notes inline. I'll fix them manually for my local tests.

janus.c

lminiero · 2021-03-25T15:50:43Z

Just tested in a few different scenarios and seems to be working as expected 👍
Merging, thanks again for your contribution! (and patience 🙂 )

lminiero reviewed Mar 16, 2021

View reviewed changes

alg force-pushed the custom_session_timeouts branch from 291e22e to 403993b Compare March 16, 2021 15:33

alg force-pushed the custom_session_timeouts branch 2 times, most recently from 9276b12 to c2073ac Compare March 22, 2021 13:51

lminiero reviewed Mar 22, 2021

View reviewed changes

alg force-pushed the custom_session_timeouts branch from c2073ac to b1bc8d0 Compare March 23, 2021 07:55

alg force-pushed the custom_session_timeouts branch from b1bc8d0 to 92936d7 Compare March 25, 2021 07:25

alg force-pushed the custom_session_timeouts branch from 92936d7 to 9e4bc5c Compare March 25, 2021 12:38

lminiero reviewed Mar 25, 2021

View reviewed changes

janus.c Outdated Show resolved Hide resolved

janus.c Outdated Show resolved Hide resolved

janus.c Outdated Show resolved Hide resolved

Added support for admin-protected custom session timeouts

773e5f3

alg force-pushed the custom_session_timeouts branch from 9e4bc5c to 773e5f3 Compare March 25, 2021 14:34

lminiero merged commit e935cdc into meetecho:master Mar 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for admin-protected custom session timeouts #2577

Added support for admin-protected custom session timeouts #2577

alg commented Mar 9, 2021

januscla commented Mar 9, 2021

lminiero commented Mar 12, 2021

alg commented Mar 12, 2021

lminiero left a comment

lminiero Mar 16, 2021

alg commented Mar 16, 2021

alg commented Mar 16, 2021

lminiero commented Mar 22, 2021

alg commented Mar 22, 2021 •

edited

Loading

lminiero left a comment

lminiero Mar 22, 2021

lminiero Mar 22, 2021

lminiero Mar 22, 2021

lminiero Mar 22, 2021

alg commented Mar 23, 2021

lminiero commented Mar 23, 2021

alg commented Mar 24, 2021

lminiero commented Mar 24, 2021

alg commented Mar 25, 2021

lminiero commented Mar 25, 2021

alg commented Mar 25, 2021

lminiero left a comment

lminiero commented Mar 25, 2021

Added support for admin-protected custom session timeouts #2577

Added support for admin-protected custom session timeouts #2577

Conversation

alg commented Mar 9, 2021

januscla commented Mar 9, 2021

lminiero commented Mar 12, 2021

alg commented Mar 12, 2021

lminiero left a comment

Choose a reason for hiding this comment

lminiero Mar 16, 2021

Choose a reason for hiding this comment

alg commented Mar 16, 2021

alg commented Mar 16, 2021

lminiero commented Mar 22, 2021

alg commented Mar 22, 2021 • edited Loading

lminiero left a comment

Choose a reason for hiding this comment

lminiero Mar 22, 2021

Choose a reason for hiding this comment

lminiero Mar 22, 2021

Choose a reason for hiding this comment

lminiero Mar 22, 2021

Choose a reason for hiding this comment

lminiero Mar 22, 2021

Choose a reason for hiding this comment

alg commented Mar 23, 2021

lminiero commented Mar 23, 2021

alg commented Mar 24, 2021

lminiero commented Mar 24, 2021

alg commented Mar 25, 2021

lminiero commented Mar 25, 2021

alg commented Mar 25, 2021

lminiero left a comment

Choose a reason for hiding this comment

lminiero commented Mar 25, 2021

alg commented Mar 22, 2021 •

edited

Loading