Re: [GTK3/PATCH] Improve multithreading support



Hi Albrecht,

On 07/23/2016 09:54:55 AM Sat, Albrecht Dreß wrote:
Hi all,

attached is a large patch which tries to streamline multithreading in Balsa.

Currently, the balsa code contains a mixture of glib's thread (and mutex and condition variable) implementation, and 
the "native" pthread one.  Furthermore, multithreading can be disabled completely (why?).

Basically, my patch attempts to (a) always use multithreading and (b) only use the glib abstraction layer instead of 
"native" pthreads.  IMO, this simplifies and improves the readability of the code.

A special case is the cancel operation (via pthread_cancel()) which GThread does not support.  However, I 
have the feeling that its usage is completely disabled anyway in Balsa:
- According to the comment in src/main.c, function balsa_cleanup(), pthread_cancel(get_mail_thread) is called 
to terminate a pending mail check thread.
- This (detached) thread is started from src/main-window.c, function check_new_messages_real(), as function 
bw_check_messages_thread().
- The very first operation of this function (also in src/main-window.c) is to call 
pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, NULL), i.e. to block all cancellation requests.  The 
cacellation state is never enabled again.
- The only place where pthread_testcancel() is called is in libbalsa/mailbox.c, at the end of the function 
libbalsa_mailbox_check(), but its completely useless as long as the cancellation state is disabled.

So I *think* removing the cancellation stuff is just safe.  However, if we need a way (do we?) to cancel a 
pending slow mailbox check operation when Balsa is closed, we need to implement it differently.  To be 
honest, I have no idea how this could be done properly...

The other changes are rather simple - just replace pthread_t by GThread, pthread_mutex_t by GMutex, and 
pthread_cond_t by GCond, and completely remove the configure option.  Please note that both 
pthread_cond_wait() and g_cond_wait() shall always be called in a loop re-checking the predicate as spurious 
wakeups may occur [1], so the current implementation is actually wrong in some places.

Furthermore, the following minor changes are included:
- libbalsa/information.h, libbalsa/libbalsa.h, src/ab-main.c, src/information-dialog.h: use the standard glib 
way to set gcc function attributes
- fix missing parentheses in a condition in function libbalsa_mailbox_messages_change_flags()

Opinions?

Cheers,
Albrecht.

Thanks for a great clean-up effort! It all works for me...

As I recall, the option to disable threads has been useful in the past, when the thread implementation had 
occasional deadlocks; I haven't used a non-threaded build in years--possible a decade! If there's no 
objection in a day or two, I'll commit it all.

Best,

Peter

Attachment: pgpOE2hXsO6Pe.pgp
Description: PGP signature



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]