Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

how to best implement asynchronous responses #131

Closed
laf0rge opened this issue Jul 30, 2019 · 20 comments
Closed

how to best implement asynchronous responses #131

laf0rge opened this issue Jul 30, 2019 · 20 comments

Comments

@laf0rge
Copy link
Contributor

laf0rge commented Jul 30, 2019

I couldn't find any forum or mailing list, so I'm filing an issue here. It's neither a bug report nor a feature request, my apologies if this is not appropriate here.

Let's say I have a (rather complex) single-threaded, event-loop driven program and I would like to add a REST API to it, preferably using ulfius. I've been brainstorming about how to do this properly.

The way how ulfius uses microhttpd (at least with modern libmicrohttpd) is to ask microhttpd to start only one thread and handle all incoming HTTP connections within that thread. At least after reading MHD source code, this is what MHD_USE_INTERNAL_POLLING_THREAD without MHD_OPTION_THREAD_POOL_SIZE appears to be doing. There's of course nothing wrong with that, but then ulfius appears to require a "synchronous" (i.e. instantaneous, non-deferred) response to be generated by the callback for a given REST API enpdoint.

This means I cannot e.g. dispatch a message to my main thread from that endpoint callback function, and pthread_cond_wait() for the main thread to process whatever it needs to do before generating a response. If I did that, I would be making the single HTTP thread of MHD to block, i.e. not process any other HTTP requests until the main thread completes. For fast responses, that might work, but for longer responses (e.g. the main thread first having to communicate with a number of other entities over various network protocols before having a response) this is not feasible.

Is three anything I'm missing? Is there any other way to asynchronously build the response?

I think if there is no other way to deal with such situations, starting microhttpd with MHD_USE_THREAD_PER_CONNECTION is one way to resolve the above issue. In this case, only the thread related to one HTTP client would block on the above-mentioned pthread_cond_wait(), while others could operate concurrently. But ulfius doesn't provide any option to do so, it will only do that unconditionally with old microhttpd versions.

@laf0rge
Copy link
Contributor Author

laf0rge commented Jul 30, 2019

Related: #27

@babelouest
Copy link
Owner

Thanks @laf0rge , you surely pointed out a new bug.

My intention is to have Ulfius multi-threaded by default. Lately I changed the options in MHD_start_daemon based on the libmicrohttpd documentation that I misread as I realize now.

I thought that MHD_USE_AUTO here was related to the threads, where it is in fact related to the polling, my bad.
(that would explain strange behaviour I noticed lately...)

I will reuse MHD_USE_THREAD_PER_CONNECTION by default.

@babelouest babelouest added the bug label Jul 30, 2019
@babelouest
Copy link
Owner

This should be fixed in the master branch now, can you validate the new behavior with the threads?
1aab28c

@babelouest
Copy link
Owner

By the way, according to this message, MHD_USE_INTERNAL_POLLING_THREAD is enforced when using threads, and if you don't set this flag explicitly, you get a warning message telling you to.

But I don't see in the documentation where MHD_OPTION_THREAD_POOL_SIZE must be set if MHD_USE_INTERNAL_POLLING_THREAD is set.

My best guess is that when you use threads, by default MHD uses a thread pool size according to the underlying system.

@laf0rge
Copy link
Contributor Author

laf0rge commented Jul 31, 2019

But I don't see in the documentation where MHD_OPTION_THREAD_POOL_SIZE must be set if MHD_USE_INTERNAL_POLLING_THREAD is set.

You don't must use it. You just get only a single thread unless you specify it. from libmicrohttpd.texi:

@item MHD_OPTION_THREAD_POOL_SIZE
@cindex performance
Number (unsigned int) of threads in thread pool. Enable
thread pooling by setting this value to to something
greater than 1. Currently, thread mode must be
MHD_USE_INTERNAL_POLLING_THREAD if thread pooling is enabled

My best guess is that when you use threads, by default MHD uses a thread pool size according to the underlying system.

unfortunately it doesn't seem like that. MHD_OPTION_THREAD_POOL_SIZE is the only place in the libmicrohttpd source that I can find where daemon->worker_pool_size is set to a value > 1.

But in any case, now that you reverted back to MHD_USE_THREAD_PER_CONNECTION (which was the default up and including to ulfius 2.5.2, for the record) I guess that particular discussion doesn't make any difference.

I'll need some time to actually develop the code that uses libulfius with pthread_cond_wait() in the callback, but I'll report back about any progress or issues.

@babelouest
Copy link
Owner

No problem, I'll wait for your feedback then, thanks

@babelouest
Copy link
Owner

babelouest commented Aug 9, 2019

Hello @laf0rge , I asked upstream the question about the connection between MHD_USE_THREAD_PER_CONNECTION and MHD_OPTION_THREAD_POOL_SIZE:
https://lists.gnu.org/archive/html/libmicrohttpd/2019-08/msg00002.html

So if one uses MHD_USE_THREAD_PER_CONNECTION, there's no need to set MHD_OPTION_THREAD_POOL_SIZE because the first option explicitly tells to open a new thread every connection.

Therefore, I think I will add an option in Ulfius 2.7 to choose between MHD_USE_THREAD_PER_CONNECTION and MHD_OPTION_THREAD_POOL_SIZE, for performance and security purpose.

@acetcom
Copy link

acetcom commented May 3, 2020

Hi @babelouest

I try to use the library in my project. Everything is good for me. However, I cannot change mhd_flags local varaible in ulfius_run_mhd_daemon() function.

I would like to use only one thread in my application as below:

/* from libmicrohttpd-0.9.70/src/examples/fileserver_example.c */
d = MHD_start_daemon (MHD_USE_ERROR_LOG,
                        atoi (argv[1]),
                        NULL, NULL, &ahc_echo, PAGE, MHD_OPTION_END);
  if (d == NULL)
    return 1;
  end = time (NULL) + atoi (argv[2]);
  while ((t = time (NULL)) < end)
  {
    tv.tv_sec = end - t;
    tv.tv_usec = 0;
    max = 0;
    FD_ZERO (&rs);
    FD_ZERO (&ws);
    FD_ZERO (&es);
    if (MHD_YES != MHD_get_fdset (d, &rs, &ws, &es, &max))
      break; /* fatal internal error */
    if (MHD_get_timeout (d, &mhd_timeout) == MHD_YES)
    {
      if (((MHD_UNSIGNED_LONG_LONG) tv.tv_sec) < mhd_timeout / 1000LL)
      {
        tv.tv_sec = mhd_timeout / 1000LL;
        tv.tv_usec = (mhd_timeout - (tv.tv_sec * 1000LL)) * 1000LL;
      }
    }
    if (-1 == select (max + 1, &rs, &ws, &es, &tv))
    {
      if (EINTR != errno)
        abort ();
    }
    MHD_run (d);
  }

If I can control mhd_flags variable, my application can be implemented with only one event loop using MDH_get_fdset(). And also, all request/response messages can be handled in an asynchronous way.

Let me know if there is other way for this.

Thank you for providing this great library.

@babelouest
Copy link
Owner

Maybe a solution for this is to allow to update the MHD_OPTIONS manually before calling ulfius_start_framework or related.

The internal function ulfius_run_mhd_daemon runs MHD_start_daemon using a built-in struct MHD_OptionItem[].

Instead of building the struct MHD_OptionItem[] inside the function ulfius_run_mhd_daemon, I can add a struct MHD_OptionItem * in the struct _u_instance, and a few dedicated functions like ulfius_append_mhd_option and ulfius_remove_mhd_option.

Most users wouldn't have to use these functions because Ulfius' default settings are good enough for most use cases, but power users could change the default settings at will.

How about that?

@acetcom
Copy link

acetcom commented May 3, 2020

Ah! It's my mistake. I can create my own MDH_Daemon instance before calling ulfius_start_framework() function. In this case, my application can use my own thread with an asynchronous way.

Thank you for your suggestion.

@babelouest
Copy link
Owner

You could do that, instead of calling ulfius_start_framework you can set your own struct u_instance.mhd_daemon with your own MHD_start_daemon return value.

Although I don't recommend to bypass internal libraries functionalities, there could be unknown side effects or breaking changes in future versions.

But it's your decision.

@acetcom
Copy link

acetcom commented May 4, 2020

There is one more question regarding this issue. In sheep_example, nb_sheep variable is accessed without mutex-lock. Is nb_sheep variable thread-safe in the callback function If I use ulfius_start_framework() without my own MHD_start_deamon().

My apologies if I was misunderstanding.

@babelouest
Copy link
Owner

@acetcom , there is no mutex in the sheep_counter example, by design. It's not even designed to be multi-users.

It's just a simple example to basically show how JSON parameters can be used in a REST API solution.

@acetcom
Copy link

acetcom commented May 4, 2020

@babelouest Ok! No Problem.
Thank you for confirming my question.

@babelouest
Copy link
Owner

I've made a new function ulfius_start_framework_with_mhd_options that should help you set your own MHD options at ulfius startup, let me know if it fit your needs. You can reopen this issue if the function doesn't behave properly.

@acetcom
Copy link

acetcom commented May 15, 2020

Hi @babelouest

My project has not only REST API but also other protocols. So I had to organize all of these into only one event-loop thread. And also, I needed microhttpd flow control.

At that time, I realized that it was difficult to use ulfius for this purpose. And I re-implemented this part in my own way.

Let me share the microhttpd wrapper as below.

https://github.com/open5gs/open5gs/blob/sbi/lib/sbi/server.h
https://github.com/open5gs/open5gs/blob/sbi/lib/sbi/server.c

I hope that it could help improve ulfius.

Thank you for considering it.
Sukchan

@babelouest
Copy link
Owner

babelouest commented May 15, 2020

No problem, but the functionality was asked by other people and may be useful to other.

Happy hacking!

@IvanRibakov
Copy link

@babelouest Thanks for doing a great job and sharing it with the community!

We've been using ulfius for a while now and as we started doing some stress testing of our API we realised that ulfius_start_framework_with_mhd_options is what we need as we would like to reduce maximum number of created threads (presumably by using MHD_OPTION_THREAD_POOL_SIZE).

Are there any estimates for when the next release is coming out so that we can take this new feature for a spin?

@babelouest
Copy link
Owner

Hello @IvanRibakov ,

Ulfius 2.6.7 will be release soon, hopefully.
I'll release new versions of my libraries at the same time when Glewlwyd 2.3 will be released as well.

@babelouest
Copy link
Owner

Hello @IvanRibakov ,

Ulfius 2.6.7 has just been released with the new features, happy hacking!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants