Creating a thread safe logging widget

[Gonzalo Garramuño]

I've tried creating a logging widget that is thread safe, but it seems
it sometimes hangs up (probably because several messages arrive
simultaneously, I guess).  The info, warning and error functions get
called by a custom C++ iostream and they can get called simultaneously
by two or three threads (like the video, audio or decode thread of my
viewer).

Here's my code, in case someone can point out an error or provide their
own version of a logging widget.


#include <cstdio>
#include <cstring>
#include <cstdlib>

#include <FL/Fl_Window.H>
#include <FL/Fl_Text_Buffer.H>
#include <FL/Fl_Text_Display.H>
#include <FL/Fl.H>
#include <FL/fl_utf8.h>

#include "core/mrvHome.h"
#include "gui/mrvLogDisplay.h"

namespace mrv {

// Style table
static
Fl_Text_Display::Style_Table_Entry kLogStyles[] = {
    // FONT COLOR      FONT FACE   SIZE   ATTR
    // --------------- ----------- ----- ------
    {  FL_BLACK,  FL_HELVETICA, 14,   0 },      // A - Info
    {  FL_DARK_YELLOW, FL_HELVETICA, 14,   0 }, // B - Warning
    {  FL_RED,    FL_HELVETICA, 14,   0 },      // C - Error
};


LogDisplay::LogDisplay( int x, int y, int w, int h, const char* l  ) :
Fl_Text_Display( x, y, w, h, l ),
_lines( 0 )
{
    color( FL_GRAY0 );

    scrollbar_align( FL_ALIGN_BOTTOM | FL_ALIGN_RIGHT );

    wrap_mode( WRAP_AT_BOUNDS, 80 );

    delete mBuffer;
    delete mStyleBuffer;
    mBuffer = new Fl_Text_Buffer();
    mStyleBuffer = new Fl_Text_Buffer();
    highlight_data(mStyleBuffer, kLogStyles, 3, 'A', 0, 0);

}

LogDisplay::~LogDisplay()
{
    delete mBuffer; mBuffer = NULL;
    delete mStyleBuffer; mStyleBuffer = NULL;
}

void LogDisplay::clear()
{
    Fl::lock();
    mStyleBuffer->text("");
    mBuffer->text("");
    redraw();
    _lines = 0;
    Fl::unlock();
    Fl::awake();
}

void LogDisplay::info( const char* x )
{
    Fl::lock();
    size_t t = strlen(x);
    char* buf = (char*)malloc( t+1 );
    buf[t] = 0;
    while( t-- )
    {
        if ( x[t] == '\n' ) {
            ++_lines;
            buf[t] = '\n';
        }
        else
        {
            buf[t] = 'A';
        }
    }
    mStyleBuffer->append( buf );
    mBuffer->append( x );
    update_v_scrollbar();
    scroll( _lines-1, 0 );

    free( buf );

Fl::unlock();
    Fl::awake();

}

void LogDisplay::warning( const char* x )
{
    Fl::lock();

    size_t t = strlen(x);
    char* buf = (char*)malloc( t+1 );
    buf[t] = 0;
    while( t-- )
    {
        if ( x[t] == '\n' ) {
            ++_lines;
            buf[t] = '\n';
        }
        else
        {
            buf[t] = 'B';
        }
    }

    mStyleBuffer->append( buf );
    mBuffer->append( x );
    update_v_scrollbar();
    scroll( _lines-1, 0 );
    free( buf );

    Fl::unlock();
    Fl::awake();

}

void LogDisplay::error( const char* x )
{
    Fl::lock();

    size_t t = strlen(x);
    char* buf = (char*)malloc( t+1 );
    buf[t] = 0;
    while( t-- )
    {
        if ( x[t] == '\n' ) {
            ++_lines;
            buf[t] = '\n';
        }
        else
        {
            buf[t] = 'C';
        }
    }

    mStyleBuffer->append( buf );
    mBuffer->append( x );
    update_v_scrollbar();
    scroll( _lines-1, 0 );
    free( buf );

    Fl::unlock();
    Fl::awake();

}

}

and the less interesting .h file:


#ifndef mrvLogDisplay_h
#define mrvLogDisplay_h

#include <atomic>

#include <FL/Fl_Text_Display.H>

namespace mrv {

class LogDisplay : public Fl_Text_Display
{
public:
    LogDisplay( int x, int y, int w, int h, const char* l = 0 );
    ~LogDisplay();

    void clear();

    void info( const char* x );
    void warning( const char* x );
    void error( const char* x );

    inline unsigned lines() const {
        return _lines;
    }

protected:
    std::atomic<unsigned int> _lines;

};

}

#endif

--
Gonzalo Garramuño
ggar...@gmail.com

[Greg Ercolano]

On 6/9/22 18:00, Gonzalo Garramuño wrote:

I've tried creating a logging widget that is thread safe, but it seems it sometimes hangs up (probably because several messages arrive simultaneously, I guess).  The info, warning and error functions get called by a custom C++ iostream and they can get called simultaneously by two or three threads (like the video, audio or decode thread of my viewer).

    When I use threads, I make sure the child thread doesn't touch anything that might be a global, and use the thread library's lock/unlock functions.

    When I have a thread interacting with the FLTK library, I tend to have the child thread and FLTK thread agree on only one common bit of data, usually a text array, and use a single lock on that array:

        > Child thread accumulates some data to append to the global array, locks it, adds the data, unlocks

        > FLTK thread uses Fl::xxx_timeout() timer to poll the above array for content, locks to read, moving data from array to widget, deletes the data read, then unlocks the array

    I don't let the child thread touch anything else that the FLTK thread might also use.
    This includes whatever file descriptors/pipes/etc that the child thread reads for its data source. All that is owned by the child, and those descriptors are unknown to the FLTK thread.

[imm]

On Fri, 10 Jun 2022 at 02:01, Gonzalo Garramuño wrote:
>
> I've tried creating a logging widget that is thread safe, but it seems
> it sometimes hangs up (probably because several messages arrive
> simultaneously, I guess). The info, warning and error functions get
> called by a custom C++ iostream and they can get called simultaneously
> by two or three threads (like the video, audio or decode thread of my
> viewer).
>
> Here's my code, in case someone can point out an error or provide their
> own version of a logging widget.
>

Hi Gonzalo,

So yeah, threads are tricky... and lock isn't really a great solution
anymore in a lot of cases. It was good in the olden times, when the
CPU's really were just a single thread of execution, and multi-threads
were simulated by switching but the actual execution was inherently
serialised.
With modern CPUs there really are many threads so the code is no
longer serialised... that gets rid of a lot of "old" types of errors,
like many deadlock cases, but then introduces a whole slew of new ones
instead...

Anyway, I'd try and make it all l work without using locks as far as
possible - I've bodged an example based on your code, attached below.
The key is to try and use the fltk awake() callback, so that all the
really hairy stuff happens in the main thread and so locks are not
required. (Of course, the awake mechanism does have some locks within
it but they are short duration and fairly "safe" in this case. I
think!)

Also, as an aside, the way you are attaching your text and style
buffers to the text_display looks wrong to me, so that may be causing
some sort of issue down the line.

TBH, I only actually tested this code on WIN32, but I expect it to
work OK with pthreads (I think you are probably on a macOS machine?)
The Makefile for the test harness might not work for macOS though,
not sure... The rest of the code should be portable though, I think.

[imm]

So, I wasn't all that happy with the way I was handling the buffer
index in the previous code, as it wasn't really thread safe. But I
couldn't be bothered doing the proper atomic updates in a
cross-platform way...

Anyway, this is better, it has the atomic updates to the buffer
pointer now, and also atomically updates the buffer in use flag. I've
tried to make this cross-platform with a few compile time
conditionals, but TBH I've only actually tried it on Windows - I
expect it to work OK on a host with intelx86 compatible operation; not
sure what the atomic intrinsics do on ARM though.

Here's the code:

[Matthias Melcher]

Modern C++ has al the thread and synchronisation calls you need in a cross platform manner.

Anyway, I would use `Fl::awake(handler, user data)` Fl::awake to send strings from any thread to the main thread. The call itself has its own lock for thread safety, and the handler is called by the main thread in the order in which `awake` was called. You just have to make sure that the handler deallocates the strings.

[Gonzalo Garramuño]

El 10/6/22 a las 16:28, 'Matthias Melcher' via fltk.general escribió:

Modern C++ has al the thread and synchronisation calls you need in a cross platform manner.

Anyway, I would use `Fl::awake(handler, user data)` Fl::awake to send strings from any thread to the main thread. The call itself has its own lock for thread safety, and the handler is called by the main thread in the order in which `awake` was called. You just have to make sure that the handler deallocates the strings.

Thank you, Matthias.  Fl::awake was just what I was looking for.  I use two global strings (one for the text and another for the style) with a mutex and I append them when I call Fl::awake on my local function.

That works better than Erco's solution of using a timeout (which I also tried with success).

-- 
Gonzalo Garramuño
ggar...@gmail.com

[Matthias Melcher]

I am glad that it works for you. I would advise against a global text buffer though. Just put everything in a class and let the caller allocate and the awake handler deallocate, for example (pseudo code, won't compile). You can add attributes and whatever else you wish to the class. No Locks or Mutex's needed:

class MyLog {

  MyLog(char *message) {

    pMessage = strdup(message);

  }

  ~MyLog() { free(pMessage);

  static void myAwakeHandler(void *userdata) {

    MyLog *This = (MyLog*)userdata;

    someTextWidget->append(This->pMessage);

    delete self;

  }

  static void send(char * message) { 

    MyLog *This = new MyLog(message); 

    Fl::awake(This->myAwakeHandler, This);

  }

  char *pMessage;

};

someThread() {

  MyLog::send("My hovercraft is full of eels."); // sends and deallocates message

}

[Gonzalo Garramuno]

> El 11 jun. 2022, a las 06:40, 'Matthias Melcher' via fltk.general <fltkg...@googlegroups.com> escribió:
>
> I am glad that it works for you. I would advise against a global text buffer though. Just put everything in a class

Thanks again for the idea. This is my final ( a tad simplified ) code for reference:



class LogData
{
public:
LogData( LogDisplay* l, const char* msg, const char s ) :
log( l ),
message( strdup( msg ) )
{
size_t t = strlen(msg);
style = (char*)malloc( t+1 );
memset( style, s, t );
style[t] = 0;
}

~LogData()
{
free( message );
free( style );
}

public:
LogDisplay* log;
char* message;
char* style;
};



void log_callback( void* v )
{
LogData* d = (LogData*) v;

LogDisplay* log = d->log;
log->style_buffer()->append( d->style );

Fl_Text_Buffer* buffer = log->buffer();
buffer->append( d->message );
log->scroll( buffer->length(), 0 );

delete d;

}

LogDisplay::LogDisplay( int x, int y, int w, int h, const char* l ) :

Fl_Text_Display( x, y, w, h, l )

{

color( FL_GRAY0 );

scrollbar_align( FL_ALIGN_BOTTOM | FL_ALIGN_RIGHT );

wrap_mode( WRAP_AT_BOUNDS, 80 );

delete mBuffer;
delete mStyleBuffer;
mBuffer = new Fl_Text_Buffer();
mStyleBuffer = new Fl_Text_Buffer();
highlight_data(mStyleBuffer, kLogStyles, 3, 'A', 0, 0);

}

LogDisplay::~LogDisplay()
{
delete mBuffer; mBuffer = NULL;
delete mStyleBuffer; mStyleBuffer = NULL;
}

void LogDisplay::clear()
{

mStyleBuffer->text("");
mBuffer->text("");
}

void LogDisplay::info( const char* x )
{

LogData* data = new LogData( this, x, 'A' );
Fl::awake( log_callback, data );

}

void LogDisplay::warning( const char* x )
{

LogData* data = new LogData( this, x, 'B' );
Fl::awake( log_callback, data );

}

void LogDisplay::error( const char* x )
{

LogData* data = new LogData( this, x, 'C' );
Fl::awake( log_callback, data );
}

}



—
Gonzalo Garramuno
ggar...@gmail.com

[Ian MacArthur]

On 12 Jun 2022, at 14:13, Gonzalo Garramuno wrote:
>
>
>> El 11 jun. 2022, a las 06:40, 'Matthias Melcher' via fltk.general <fltkg...@googlegroups.com> escribió:
>>
>> I am glad that it works for you. I would advise against a global text buffer though. Just put everything in a class
>
> Thanks again for the idea. This is my final ( a tad simplified ) code for reference:

<snip>

So, far be it from me to disagree with Matt (that’s code for “I am about to disagree with Matt”, BTW) but I’m not hugely keen on dynamically creating/deleting objects to buffer the “transient” buffer objects that are passed asynch from the worker thread to main.

There reason is that, at least historically anyway, doing so entailed a trip through the memory allocator, which was a serialization point and tended to mess up my threads.
Now, it is entirely possible that the modern OS memory allocators are fully multi-thread aware and so do not cause a serialization when used like this, but I am old now and do not know!

YMMV...

[Greg Ercolano]

FWIW, I have a threaded example of a terminal emulator which uses a combo of Fl_Text_Editor and separate read/write pipes to interact with the shell, each pipe has its own child thread in addition to the main FLTK thread:
http://seriss.com/people/erco/fltk/unix-bidir-dumb-terminal.cxx

It's unix specific code (mac+linux) because of the use of pthreads and unix style pipes. There are direct windows equivalents that'd probably be relatively easy to swap in, just WIN32 APIs for properly mixing pipes with subprocesses (for the DOS shell) is a bit of annoying API work.

[Matthias Melcher]

Ian MacArthur schrieb am Sonntag, 12. Juni 2022 um 19:29:45 UTC+2:

So, far be it from me to disagree with Matt (that’s code for “I am about to disagree with Matt”, BTW) but I’m not hugely keen on dynamically creating/deleting objects to buffer the “transient” buffer objects that are passed asynch from the worker thread to main.

As long as you don't use special allocators or memory pools, allocators share the same heap, and malloc() and free() are thread-safe on modern operating systems (on Linux for at least 14 years since glib-2.20).

[Gonzalo Garramuno]

> El 12 jun. 2022, a las 14:29, Ian MacArthur <imaca...@gmail.com> escribió:
>
> Now, it is entirely possible that the modern OS memory allocators are fully multi-thread aware and so do not cause a serialization when used like this, but I am old now and do not know!
>

The new and delete operators are thread safe. Malloc is safe depending on OS and compiler flags. On modern Unix, it is thread safe. On Windows, it is thread safe if compiled with one of the multithreaded flags (like /MD).
Or at least that’s what stack overflow said :D

!https://stackoverflow.com/questions/855763/is-malloc-thread-safe

—
Gonzalo Garramuno
ggar...@gmail.com

[Ian MacArthur]

On 12 Jun 2022, at 18:40, Gonzalo Garramuno wrote:
>
>
>> El 12 jun. 2022, a las 14:29, Ian MacArthur <imaca...@gmail.com> escribió:
>>
>> Now, it is entirely possible that the modern OS memory allocators are fully multi-thread aware and so do not cause a serialization when used like this, but I am old now and do not know!
>>
>
> The new and delete operators are thread safe. Malloc is safe depending on OS and compiler flags. On modern Unix, it is thread safe. On Windows, it is thread safe if compiled with one of the multithreaded flags (like /MD).
> Or at least that’s what stack overflow said :D

Sorry, I was not clear: I have no doubt they are thread-safe, rather what I do not know is how that safety is achieved.
Historically it was achieved by having a "global lock" on the heap, which had the effect that if multiple threads were allocating/deallocating they would be serialized at that point, breaking the multi-thread nature of the operations.

Now, there are lockless ways this can be done, and it is entirely feasible that the modern allocators do that (I think that is what Matt is saying) but certainly in the past the allocators were rendered thread-safe by dint of using what was essentially a master lock, and I have seen this have “unfortunate” side effects that badly perturbed the multi-thread nature of the runtime code.
So I try to avoid that (which is why I, I suppose, I don’t know whether it is even true or not now!)

[Bill Spitzak]

Modern ones I think use (small) per-thread heaps. The problem then is that allocating and freeing in different threads loses the multithreading advantage of this and may result in locking anyway.

--
You received this message because you are subscribed to the Google Groups "fltk.general" group.
To unsubscribe from this group and stop receiving emails from it, send an email to fltkgeneral...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/fltkgeneral/F1C78245-B245-4CF6-9E04-6D0C8D9222C0%40gmail.com.

[Ian MacArthur]

On Sunday, 12 June 2022 at 23:37:10 UTC+1 spitzak wrote:

Modern ones I think use (small) per-thread heaps. The problem then is that allocating and freeing in different threads loses the multithreading advantage of this and may result in locking anyway.

OK, that's interesting. I haven't looked at this aspect in a good while so had no idea what was really being done in practice now.

I can see a per-thread heap being quite a neat way to duck the  issue and that probably covers the majority of typical use cases.

But in this specific case, allocating in one thread then freeing in a different thread, it does seem like that will lead to some sort of synchronization, so I guess a lock does seem likely then.

Which is a pity, as there are ways this could be made fully lock-free; though I admit they would probably be unnecessarily complex for the vast majority of cases!