FLTK logo

STR #3223

FLTK matrix user chat room
(using Element browser app)   FLTK gitter user chat room   GitHub FLTK Project   FLTK News RSS Feed  
  FLTK Apps      FLTK Library      Forums      Links     Login 
 Home  |  Articles & FAQs  |  Bugs & Features  |  Documentation  |  Download  |  Screenshots  ]
 

Return to Bugs & Features | Roadmap 1.3 | SVN ⇄ GIT ]

STR #3223

Application:FLTK Library
Status:1 - Closed w/Resolution
Priority:2 - Low, e.g. a documentation error or undocumented side-effect
Scope:3 - Applies to all machines and operating systems
Subsystem:Core Library
Summary:Fl::awake() and Fl::thread_message() need fixes and better documentation
Version:1.3.3
Created By:AlbrechtS
Assigned To:AlbrechtS
Fix Version:1.3.4 (SVN: v10714)
Update Notification:

Receive EMails Don't Receive EMails

Trouble Report Files:


Name/Time/Date Filename/Size  
 
#1 ianmacarthur
07:23 Oct 21, 2015
advanced.dox
16k
 
 
#2 AlbrechtS
03:58 Oct 22, 2015
typos.diff
2k
 
 
#3 ianmacarthur
07:55 Oct 22, 2015
advanced_v2.dox
17k
 
 
#4 ianmacarthur
04:31 Oct 23, 2015
advanced_v3.dox
17k
 
     

Trouble Report Comments:


Name/Time/Date Text  
 
#1 AlbrechtS
04:55 Apr 22, 2015
This is a reminder to check some questionable code.

(1) In src/Fl_lock.cxx, line #84, we have:

  if (awake_ring_head_==awake_ring_tail_-1 || awake_ring_head_+1==awake_ring_tail_) {
    // ring is full. Return -1 as an error indicator.

This should probably be:

  if (awake_ring_head_+1==awake_ring_tail_ ||
      awake_ring_head_+1-awake_ring_size_==awake_ring_tail_) {
    // ring is full. Return -1 as an error indicator.

Note that all involved variables are signed (int). In the original code the two or'd conditions are equialent. In the modified code the first condition accounts for "normal" conditions, where both indices are within the normal array range, whereas the second condition is true in the "wrapping" case:

(awake_ring_head_ == awake_ring_size_ -1) && (awake_ring_tail_ == 0).

In the unlikely case that the second condition is true the old code would "wrap" the head index and hence lose all (1023) ring buffer entries.
 
 
#2 AlbrechtS
05:08 Apr 22, 2015
(2) Fl::thread_message() needs better documentation and the current code is questionable.

In src/Fl_win32.cxx, line #423 we have:

    if (fl_msg.message == fl_wake_msg) {
      // Used for awaking wait() from another thread
      thread_message_ = (void*)fl_msg.wParam;
      Fl_Awake_Handler func;
      void *data;
      while (Fl::get_awake_handler_(func, data)==0)
        func(data);
    }

This seems to set the global 'thread_message_' which is returned by Fl::thread_message() only once before _all_ queued awake handler functions are called. This looks like code that was valid before the queueing of awake messages with awake functions was added ... or something like that. Needs investigation.

This should also be checked/verified for non-Windows OS'es.

Maybe Fl::thread_message() should be deprecated in favor of using

  Fl::awake(Fl_Awake_Handler cb, void* message = 0);

Note that Fl::thread_message() clears the global 'thread_message_' whenever it is called, but it is probably not used anyway whenever user code uses the new awake(Fl_Awake_Handler cb, void* message = 0) method.

Docs should be checked and clarified.
 
 
#4 AlbrechtS
05:17 Apr 22, 2015
(3) Fl::awake_ring_head_, Fl::awake_ring_tail_, Fl::awake_ring_size_ etc. should be private. These member variables are currently public.

This might break the ABI though and should be done in 1.4.0.
 
 
#5 ianmacarthur
13:43 Apr 22, 2015
Responding to post #4:

This is true, though my proposed "fix" for #3143 actually depends on them being public for now, so I'd be in favour of leaving them alone for now.

And I guess they are exposed in the ABI, though I doubt they are used much!

In 1.4.x we could implement my "fix" by adding a "check ring buffer" method in FL_lock.cxx, rather than doing it in fl_win32.cxx as my current plan does...
 
 
#6 ianmacarthur
14:00 Apr 22, 2015
Responding to #1:

Looking at this code, I think you are correct.

However, when I looked at this code yesterday, I did not notice; this way of testing a circular buffer is quite a common pattern and I've seen it before, so it looked "normal" here.

But, the "usual" way I've seen this used is to make the queue length a power of two (we have 1024 here) and then mask the indices before they are tested (with (1024 - 1) i.e. 0x3FF in this case).

If you do that, ((tail - 1) & 0x3FF) can be very different from ((head + 1) & 0x3FF).

If we added the masks in, this code would likely be correct then I think.

I wonder if it used to have the masks, but they got stripped out at some point?


Responding to #2:

I think this is actually "correct", in so far as it goes.

The docs say that thread_message_ holds the last message that was posted, and that older ones are lost, so in this case we have thread_message_ holding only the message from the latest PostThreadMessage call that was received, regardless of how many other messages have been written into the ring buffer queue.

If there are multiple PostThreadMessage events pending, we will read them all in turn, and thread_message_ will end up with the value of the last one processed.

Also, note that with my proposed fix to #3143, this will not happen at all for the "lost" thread messages, so thread_message_ will still be left holding the last PostThreadMessage that we actually received, even if several others have been lost in the meantime - only the callbacks, which we are putting in our own queue, are "guaranteed" to be delivered...
 
 
#7 ianmacarthur
14:38 Apr 22, 2015
I propose we fix #1 as follows:

/** Adds an awake handler for use in awake(). */
int Fl::add_awake_handler_(Fl_Awake_Handler func, void *data)
{
  int ret = 0;
  lock_ring();
  if (!awake_ring_) {
    awake_ring_size_ = AWAKE_RING_SIZE;
    awake_ring_ = (Fl_Awake_Handler*)malloc(awake_ring_size_*sizeof(Fl_Awake_Handler));
    awake_data_ = (void**)malloc(awake_ring_size_*sizeof(void*));
    // explicitly initialize the head and tail indices
    awake_ring_head_= awake_ring_tail_ = 0;
  }
  int next_head = awake_ring_head_ + 1;
  if (next_head >= awake_ring_size_) {
    next_head = 0;
  }
  if (next_head == awake_ring_tail_) {
    // ring is full. Return -1 as an error indicator.
    ret = -1;
  } else {
    awake_ring_[awake_ring_head_] = func;
    awake_data_[awake_ring_head_] = data;
    awake_ring_head_ = next_head;
  }
  unlock_ring();
  return ret;
}

Thoughts?
 
 
#8 ianmacarthur
03:17 Apr 23, 2015
The proposed adjustment to the index test in add_handler were applied to thesvn repo at r10714.

Other issues remain.
Tetsing ofthe applied fix indiocate that it appears to be robust.

Note that user code needs to check whether the Fl::awake(cb, etc) call succeeds or not, if they want to be sure that adding a callback worked (this is obvious of course, but my test harness initially did not check and I wondered where the missed cb's had gone when the buffer was full! Doh!)
 
 
#9 AlbrechtS
04:31 Apr 23, 2015
#1: Thanks for the fix. I like the way you changed the test by introducing next_head.

#4,#5: Making these member variables private should be done in 1.4.0. We need to add one or more accessor methods (read-only getter's). The fact that a lock is used internally makes it IMHO _necessary_ to prohibit direct access. WRT ABI: if anybody really used these variables we must break that code. Really. We can still think about which getter methods we need to help.
 
 
#10 AlbrechtS
05:24 Apr 23, 2015
#2, #6: I didn't have the time to investigate further, so my intention was to post a reminder with what I had found out so far so that it doesn't get lost.

However I believe that the code does not do what the documentation says. If you post multiple thread messages with Fl::awake(msg) and if we assume that they are queued in the Windows message queue, then the _first_ message retrieved will set the thread_message_ and process all queued awake messages in the ring buffer. In fact this could even overwrite (clear) existing contents of thread_message_ if a thread message was posted before and not yet read. Following buffered messages in the Windows message queue will overwrite the global thread_message_ variable but find the internal message queue empty (flushed by the first message).

Example calling Fl::awake(..) in user thread(s), i.e. maybe in different parallel threads:

  Fl::awake(msg1);

  ... FLTK event loop processes this and sets: thread_message_ = msg1;
  ... assume user code does not use Fl::thread_message();
  ... before the event loop continues (see below)

  Fl::awake(cb,data1); // queues handler with 'data1' userdata
  Fl::awake(cb,data2); // dto. with data2 ...
  Fl::awake(msg2);     // msg2 overwrites msg1, but will be lost
  Fl::awake(cb,data3); // queues handler with 'data3' userdata
  Fl::awake(cb,data4); // dto. with data4 ...

  ... FLTK event loop processes messages:

  Fl::awake(cb,data1) sets thread_message_ = 0; (overwrites msg2, since Fl::awake() posted 0).

  FLTK processes all queued handlers with data1, data2, ... resp.

Conclusion: Fl::awake(msg) and Fl::awake(cb,data) are not compatible and should not be used together. Anyway, I'd go so far to say that Fl::awake(void *msg) is deprecated, since we have better ways and its use is highly unreliable (as the documentation states anyway: messages may be lost).

Anyway, you may still say "it's all documented", but I believe we can do better.


As for the documentation: in chapter "Advanced FLTK" we have as the first and prominent example for "multithreaded applications":

int main() {
  Fl::lock();
  /* run thread */
  while (Fl::wait() > 0) {
    if (Fl::thread_message()) {
      /* process your data */
    }
  }
}

For new users reading the docs this implies that you should use Fl::wait() and Fl::thread_message() to use multithreaded applications. Later we have:

"You can also tell the main thread..." with a callback example:

  Fl::awake(do_something, data);  // call something in main thread

IMHO this implies that the latter is the second best choice (at least for a new reader).


My suggestion:

This _entire_ chapter should be rewritten (at least reordered and reworded). We should emphasize that

  Fl::awake(cb, data);

is the primary way to call Fl::awake(..) with any kind of userdata (data == msg). This is the new, modern, best, and the only reliable way.

Calling Fl::awake() w/o any message (i.e. NULL) serves the sole purpose to awake the FLTK event loop. This is also documented somewhere to be the only way to effect a redraw(). As a side effect it overwrites the global thread_message_ with 0 (NULL).

Calling Fl::awake(void *msg) with a non-null 'msg' value sets and may overwrite the global thread_message_ variable (Fl::thread_message()) in an inherently unreliable way and is deprecated. We should probably remove the old and deprecated example shown above with 'while (Fl::wait() > 0) {..}' to discourage users to use Fl::thread_message() at all.

Oh, yes, and Fl::thread_message() is deprecated as well.

I'm not very good at writing docs, particularly about threads, so if anybody else would take that job I wouldn't mind. ;-)

I hope that my thoughts may help and improve the docs.
 
 
#11 AlbrechtS
05:45 Oct 19, 2015
Code is fixed since svn r 10714 (thanks to Ian).

Documentation should still be fixed, but that is only low priority (changed priority from high to low).

@Ian: if you want to take ownership of this STR and fix the docs, please feel free to do so...
 
 
#12 ianmacarthur
07:25 Oct 21, 2015
Assuming it didn't get mangled in the post, I have posted a revised version of advanced.dox.

I'd appreciate comments, feedback, corrections. I have stared at this so long now I can't see the mistakes any more.

Tested with doxygen 1.8.8 (what I have on this machine) and it appears to be OK but...
 
 
#13 AlbrechtS
03:58 Oct 22, 2015
I'll post more comments in fltk.coredev, but so far: awesome work, clear and understandable, AFAICT. Thank you very much!

See attached file typos.diff for some minor fixes. One of them concerns the old text but should be fixed as well ("UTF-8").
 
 
#14 ianmacarthur
07:56 Oct 22, 2015
Update advanced.dox file to incorporate Albrecht's suggestion form coredev discussion.  
 
#15 manolo
08:39 Oct 22, 2015
Great. These explanations will be very helpful.

I have a small suggestion concerning the 1st item
(Don't show() or hide()...) of the last section
(FLTK multithreaded Constraints):

The 1st sentence could mention explicitly that worker
threads are concerned:
Don't show() or hide() from a worker thread anything...

and the 3rd sentence (It is advised never to call...)
could be removed altogether because it brings no
new information.
 
 
#16 ianmacarthur
04:33 Oct 23, 2015
New version of advanced.dox, that I hope addresses Manolo's points, at least well enough for us to commit this text for now!

Feedback welcomed!
 
 
#17 AlbrechtS
13:15 Oct 23, 2015
+1 to commit v3.

I also checked doxygen doc generation and HTML and PDF docs. Everything looks good.

Great, thanks.
 
 
#18 AlbrechtS
11:58 Sep 13, 2016
FTR: I see that advanced_v3.dox has been committed in svn r10874 on Oct 23, 2015.

That said, I believe that this STR can be closed:

The docs are improved and the unreliability of intermixing different threading approaches, i.e. different uses of Fl::awake(..) and Fl::thread_message() is documented.

The only remaining points are the public member variables mentioned in comments #4, #5, and #9 that should be private. This should be addressed in another STR for FLTK 1.4.0.

Posted STR #3327: http://www.fltk.org/str.php?L3327

Any reasons not to close /this/ STR ?
 
 
#19 ianmacarthur
02:32 Sep 14, 2016
I concur: I think we can close this one, and roll the ABI changing global variables forward into 1.4.x for fixing.

AFAICT these changes are working OK in 1.3.x now, so I'm happy that the actual implementation works as needed.
 
 
#20 AlbrechtS
02:43 Sep 14, 2016
Thanks for your quick reply.

Closing this STR now.
 
     

Return to Bugs & Features ]

 
 

Comments are owned by the poster. All other content is copyright 1998-2024 by Bill Spitzak and others. This project is hosted by The FLTK Team. Please report site problems to 'erco@seriss.com'.