Converted HTTP transport plugin to single thread

[lminiero]

As the title says, I've refactored the HTTP transport plugin to have it work in single thread mode.

The way it worked so far was using multiple threads, and the default was actually to use one thread per connection. As you can imagine, this is a way less than optimal way of handling HTTP traffic, especially if many users are connected at the same time: while it could make sense to have a thread for long polling, a dedicated thread for handling a keepalive or trickle request were overkill instead. We had a configuration property to limit the number of threads in the pool (e.g., "use 100 threads" instead of infinite), but that had many problems of its own: if 99 or 100 long polls kept all the threads busy, the system would become unresponsive until they were freed.

This new PR was written to fix all that. It basically make use of the suspend/resume mechanism that libmicrohttpd (the library the HTTP transport uses) exposes, to use a single thread for each web server instead. This means that, while before you could have a ton of threads spawned, here the max you can get is 4 (if you enable both Janus and Admin backends, and both HTTP and HTTPS). There is an additional thread to handle timeouts (as timeouts for suspended connections are not handled by the library itself) but again, much less than what we were doing before.

I've made some tests and it seems to be working as expected for me. I had to fight with some freezes, but I eventually found out they were caused by the version of libmicrohttpd I had installed (0.9.58). As soon as I updated (0.9.59) the freezes disappeared, and since they appeared often and quite early I'm confident they're gone for good (besides, the libmicrohttpd changelog mentions such a fix as well).

Please test as much as you can and let me know if you encounter any weird behaviour or regression.

[amnonbb]

I tried it on Centos 7
and get 2 from 4 cores 100% loaded all the time.
In log did not see something unusual.
Janus work behind nginx proxy.

[lminiero]

What makes you think the cause is the web server? Which thread keeps the core(s) busy?

[amnonbb]

https://imgur.com/a/XtKAC
libmicrohttpd-0.9.33-2.el7.x86_64

[lminiero]

As explained in the description, this needs a recent libmicrohttpd (0.9.59) to work properly. Please try with that and let me know if it's still an issue.

[amnonbb]

I tried with libmicrohttpd (0.9.59)
issue is gone

[mtdxc]

is this a bug?
sounds like need to move events to out of while and add events++

[lminiero]

Good catch, this was a copy/paste from the original code, which didn't have a while there. I'll fix it shortly.

[mtdxc]

i merge this pr in one of our server and and run two or three hours, will got a coredump, this is the bt

#0  0x00007fbc37b4fca9 in g_source_attach ()
   from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#1  0x00007fbbfd765b41 in janus_http_handler (cls=<optimized out>, 
    connection=0x7fbc180db610, 
    url=0x7fbc1823b065 "/janus/3386795195003419/672857113052025", 
    method=<optimized out>, version=<optimized out>, upload_data=0x0, 
    upload_data_size=0x7fbbfcd39038, ptr=0x7fbc180db668)
    at transports/janus_http.c:1405
#2  0x00007fbbfd541786 in call_connection_handler (
    connection=connection@entry=0x7fbc180db610) at connection.c:2284
#3  0x00007fbbfd5432a0 in MHD_connection_handle_idle (
    connection=connection@entry=0x7fbc180db610) at connection.c:3486
#4  0x00007fbbfd544e34 in call_handlers (con=con@entry=0x7fbc180db610, 
    read_ready=<optimized out>, write_ready=<optimized out>, 
    force_close=<optimized out>) at daemon.c:1113
#5  0x00007fbbfd5494ca in MHD_epoll (daemon=daemon@entry=0x1515e00, 
    may_block=may_block@entry=1) at daemon.c:4265
#6  0x00007fbbfd54a44a in MHD_polling_thread (cls=0x1515e00) at daemon.c:4427
#7  0x00007fbc36582182 in start_thread (arg=0x7fbbfcd3a700)
    at pthread_create.c:312
#8  0x00007fbc362af47d in clone ()
    at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111

it crash in janus_http_handler

parsingdone:
	/* Check if we have session and handle identifiers */
	session_id = session_path ? g_ascii_strtoull(session_path, NULL, 10) : 0;
	handle_id = handle_path ? g_ascii_strtoull(handle_path, NULL, 10) : 0;
	if(session_id > 0)
		json_object_set_new(root, "session_id", json_integer(session_id));
	if(handle_id > 0)
		json_object_set_new(root, "handle_id", json_integer(handle_id));

	/* Suspend the connection and pass the ball to the core */
	JANUS_LOG(LOG_HUGE, "Forwarding request to the core (%p)\n", msg);
	gateway->incoming_request(&janus_http_transport, msg, msg, FALSE, root, &error);
	response = MHD_create_response_from_callback(MHD_SIZE_UNKNOWN,
		500, &janus_http_response_callback, msg, NULL);
	if(response == NULL) {
		ret = MHD_NO;
	} else {
		g_atomic_int_set(&msg->suspended, 1);
		MHD_suspend_connection(msg->connection);
		MHD_add_response_header(response, "Content-Type", "application/json");
		janus_http_add_cors_headers(msg, response);
		ret = MHD_queue_response(msg->connection, MHD_HTTP_OK, response);
		MHD_destroy_response(response);
		/* We won't wait forever for an answer (about 10 seconds) */
		msg->timeout = g_timeout_source_new_seconds(10);
		g_source_set_callback(msg->timeout, janus_http_timeout, msg, NULL);
		// crash on this code
		**g_source_attach(msg->timeout, httpctx);**
	}

any help about this, by the way i use libmicrohttpd-0.9.59
i have found the problem:
g_timeout_source_new_seconds return null...
does it need to use another timer implement?

[mtdxc]

I have found the problem
move g_timeout_source_new_seconds before gateway->incoming_request just fixed the bug
when gateway async process request in other thread , and thread switch after g_timeout_source_new_seconds called and then call janus_http_send_message to remove msg->timeout and thread switch back may cause this problem msg->timeout to null...

	/* We won't wait forever for an answer (about 10 seconds) */
	msg->timeout = g_timeout_source_new_seconds(10);
	g_source_set_callback(msg->timeout, janus_http_timeout, msg, NULL);
	g_source_attach(msg->timeout, httpctx);**

	/* Suspend the connection and pass the ball to the core */
	JANUS_LOG(LOG_HUGE, "Forwarding request to the core (%p)\n", msg);
	gateway->incoming_request(&janus_http_transport, msg, msg, FALSE, root, &error);
	response = MHD_create_response_from_callback(MHD_SIZE_UNKNOWN,
		500, &janus_http_response_callback, msg, NULL);
	if(response == NULL) {
		ret = MHD_NO;
	} else {
		g_atomic_int_set(&msg->suspended, 1);
		MHD_suspend_connection(msg->connection);
		MHD_add_response_header(response, "Content-Type", "application/json");
		janus_http_add_cors_headers(msg, response);
		ret = MHD_queue_response(msg->connection, MHD_HTTP_OK, response);
		MHD_destroy_response(response);
	}

[atoppi]

@mtdxc Good catch!
This is likely the same crash experienced in last experiments in #1154 issue.
We'll analyze your changes and eventually fix the PR.
Thank you.

[lminiero]

@mtdxc for next time, please DON'T paste huge bunches of text inline, or use a code block like you did in the second message: because of the gdb debugging, your first message had broken references to issues 1, 2, etc. I fixed it manually for you.

[mtdxc]

@lminiero ok, @atoppi nice job...

[lminiero]

Just noticed we never aligned this branch to the new master (refcount), working on that now...

[mtltechtemp]

@lminiero Hey, i think you can merge it.
I tested it for two hours and it's running flawlessly well.

[lminiero]

@mtltechtemp thanks for the feedback! One of the reasons why this hasn't been merged yet, though, is that it requires MHD 0.9.59 which may not yet be the default on most repos.

[mtltechtemp]

@lminiero Just put a warning if the version is not the right one during compilation ;)

[tmatth]

@mtltechtemp thanks for the feedback! One of the reasons why this hasn't been merged yet, though, is that it requires MHD 0.9.59 which may not yet be the default on most repos.

Maybe now? We're up to 0.9.62-1 in Debian Stable: https://packages.debian.org/buster/libmicrohttpd-dev

[lminiero]

@tmatth thanks for the heads up! I'll notify this on the group, so that people have a final chance to test it.

[eLvErDe]

Hi guys,

We're pretty interested in seeing if this fix would finally get rid of temporary locks of streaming API that still occurs sometimes (related to remote stream being unreachable or reconnecting, we ended up implementing blacklist of such streams that reduce drastically API locks).

Are you planning to merge it soon ?

Adam

[lminiero]

@eLvErDe yes, the plan is to merge very soon, as we did give people heads-up and a lot of time to make sure it works as expected for them. Expect this to land next week or the one after that.

[eLvErDe]

Thanks I'll probably build a test version with the patch for the moment so I can start testing

[eLvErDe]

Hi,

This is very promising. I built 0.9.0 with this patch and couldn't not produce an API lock during intensive testing.
Moreover I have the feeling streaming create request are faster.

Adam

[lminiero]

I could have waited two more days, as this pull request was created almost exactly two years ago (the 12th of March 2018), but I think we waited long enough... 😉
Finally merging!