| [ 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: | |
Trouble Report Files:
Trouble Report Comments:
|
#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 ]
|
| |