onion without pthreads support (and use of mutex inside the onion data structure)

[Basile Starynkevitch]

Hello all,

Hello All,

Do we care about no pthreads support at compile time?

I would prefer to suppose that we always HAVE_PTHREADS. That would make the code much more readable. I dream of removing all the #ifdef HAVE_PTHREADS in the code, and suppose that we do have Pthreads at compile time. I cannot name a Linux distribution not having pthreads. Today, every libc ships with them.

It is 2018. Onion is mostly POSIX targetted. Do we still care today about POSIX systems without pthreads support? Even cheap computers have threads, and most of them have two cores at least (ok cheap Rasperry Pi Zero W are single core, but a Linux kernel on them could run a few threads). My feeling is that people coding for these ultracheap devices either will use a Linux kernel capable of multithreading, or install some IoT micro OS like Contiki -on which onion cannot run-.

Of course, we still have a mode where people won't use O_THREADED at onion_new time. But that happens at runtime.

BTW; I think that most public routines in onion.c should systematically use
#ifdef HAVE_PTHREADS
    pthread_mutex_lock(&server->mutex);
#endif /*HAVE_PTHREADS*/

near their beginning

and the corresponding
#ifdef HAVE_PTHREADS
    pthread_mutex_unlock(&server->mutex);
#endif /*HAVE_PTHREADS*/

near their end.

David and all, I believe that all public functions in onion.h should be thread-friendly (since they could be called by some application thread, outside of the event loop thread), unless explicitly documented as thread-unsafe (this is not the case in latest commit 99fd33d5825666df9). So they need both calls to pthread_mutex_lock & pthread_mutex_unlock. If there are not thread-friendly, we should document that explicitly (in a doxygen-ed comment) and perhaps adopt some naming convention (maybe a suffix like _noth). I just committed src/onion/onion.c in my onion, commit d17c05ce4708278 but this is untested code (I just have added systematically these pthread_mutex_lock & pthread_mutex_unlock like above). I just checked that it compiles. Could you please tell me if I am on the right track? What public functions from onion.h do you expect to be callable after the event loop (so after start of onion_listen)? What public functions on onion* do you expect to be called from other threads (application-specific ones).

Perhaps some functions in onion.h are supposed to be callable inside requests processing. Then we probably need the mutex to be recursive (and that makes it slower). Otherwise, we need to document which onion.h functions cannot be called by request processing. What did you decide?

In other words, do we want an application accept some POST request to do some onion_set_max_threads ? I guess that not, but then we should document that. Otherwise, the mutex needs to be recursive (so a bit slower).

BTW, in my scenario (bismon) almost all HTTP requests are actually processed elsewhere than in Onion-related threads. The basic scenario is : add some "agenda task" to be processed by some existing thread pool (outside of onion) and wait till that thread pool is pthread_cond_notify-ing that my view of the HTTP exchange has completed. Then the onion thread is sending the response back (to the browser).

Regards.

-- 
Basile STARYNKEVITCH   == http://starynkevitch.net/Basile
opinions are mine only - les opinions sont seulement miennes
Bourg La Reine, France

[Basile Starynkevitch]

On 07/03/2018 11:41 PM, Basile Starynkevitch wrote:

David and all, I believe that all public functions in onion.h should be thread-friendly (since they could be called by some application thread, outside of the event loop thread), unless explicitly documented as thread-unsafe (this is not the case in latest commit 99fd33d5825666df9). So they need both calls to pthread_mutex_lock & pthread_mutex_unlock. If there are not thread-friendly, we should document that explicitly (in a doxygen-ed comment) and perhaps adopt some naming convention (maybe a suffix like _noth). I just committed src/onion/onion.c in my onion, commit d17c05ce4708278 but this is untested code (I just have added systematically these pthread_mutex_lock & pthread_mutex_unlock like above). I just checked that it compiles. Could you please tell me if I am on the right track? What public functions from onion.h do you expect to be callable after the event loop (so after start of onion_listen)? What public functions on onion* do you expect to be called from other threads (application-specific ones).

Another approach could be to document every public function of onion.h as thread-unfriendly, explicitly document that applications should just set up their onion at initialization before calling onion_listen (as most applications do) and explain that changing the onion should not be done inside HTTP request handlers. We might in addition provide a function to do something under's onion lock, e.g.

typedef void* onion_work_sigt(onion*server, void*data);

/* this function runs workrout protected by the onion's mutex;
   you need to use that inside any request handlers which modify or access the onion */
void* onion_do_locked(onion*server, onion_work_sigt* workrout, void*clientdata) {
   void* res = NULL;
   pthread_mutex_lock(&server->mutex);
   res = (*workrout)(server, clientdata);
   pthread_mutex_unlock(&server->mutex);
   return res;
}

Still another approach is to give public access to locking and unlocking routines

void onion_lock(onion*server) { pthread_mutex_lock(&server->mutex); }
void onion_unlock(onion*server) { pthread_mutex_unlock(&server->mutex); }

What do you think?

Cheers

[David Moreno Montero]

Hi,

as you said its only safe to do something before initialization. I would start making it more clear.

I would add  the "onion_lock/onion_unlock" option.

As a rule of thumb all functions that initialize the state (onion_new, flags, onion_url*...) are not thread safe. All functions that are to be used normally on a request handler are thread safe by virtue of not being shared (so if you create your own threads in that function, you may not use them freely).

Anyway I would not add locks everywhere, as for example, the initialization of urls normally is made in only one thread, and once set, not changed. And then it is read from many threads to call the proper handler. Adding a mutex or rwlock there would heavily penalize the hot path.

A solution to avoid problems is to do not allow those changes: if onion is in listen mode (an atomic flag which can be checked without holding locks) onion functions that change internal state will always fail.

I will check which function fall in this category a get back later with more info.

Anyway, for your project, Basil, maybe I'm wrong, but you dont need to get into this details. Or do you need to change any onion state from your handlers?

Regards,

David

--

David Moreno Montero 

dmo...@coralbits.com
+34 658 18 77 17

http://www.coralbits.com

[Basile Starynkevitch]

On 2018-07-04 10:19, David Moreno Montero wrote:
> Hi,
>
> as you said its only safe to do something before initialization. I
> would start making it more clear.
>
> I would add the "onion_lock/onion_unlock" option.

Should we keep the HAVE_PTHREADS, or assume (like I do) that we have
pthreads everywhere?

Cheers.

--
Basile Starynkevitch http://starynkevitch.net/Basile
France (opinions are only mine)

[David Moreno Montero]

Ideally there should be onion_low_pthread_xx or something that could be empty if not available... But realistically, I agree that we can assume there is always pthreads, so we can just remove all the #ifdefs.

Anyway whatever has less impact: if its only add the ifdef at the onion_lock/unlock, so be it, as it will have less impact than removing all the ifdefs. If it needs many new ifdefs, better remove the current ones.

Regards,

David.

[Basile Starynkevitch]

On 07/04/2018 11:47 AM, David Moreno Montero wrote:
> Ideally there should be onion_low_pthread_xx or something that could
> be empty if not available... But realistically, I agree that we can
> assume there is always pthreads, so we can just remove all the #ifdefs.
>
> Anyway whatever has less impact: if its only add the ifdef at the
> onion_lock/unlock, so be it, as it will have less impact than removing
> all the ifdefs. If it needs many new ifdefs, better remove the current
> ones.

The HAVE_PTHREADS removal mostly will improve readability.

I agree that in the long term, onion_low_thread_xx could be better.
Don't have time for that now.

[Basile Starynkevitch]

On 07/04/2018 01:09 PM, Basile Starynkevitch wrote:

The HAVE_PTHREADS removal mostly will improve readability.

BTW, it is easily automated with unifdef, see https://dotat.at/prog/unifdef/ for more.

What I am unable to do is improve the cmakefile machinery (I really hate cmake, and I cannot afford learning it). I would leave that step to you. So I committed in my commit 7fdb02c818748b29bd177dee63 the removal of HAVE_PTHREADS.

I'm hoping that the code is now easier to read.

Cheers.