bugGNU Octave - Bugs: bug #53513, graphics.cc-tst FAIL

 
 

bug #53513: graphics.cc-tst FAIL

Submitted by:  Dmitri A. Sergatskov <dasergatskov>
Submitted on:  Thu 29 Mar 2018 05:00:14 PM UTC  
 
Category: InterpreterSeverity: 3 - Normal
Priority: 5 - NormalItem Group: None
Status: PostponedAssigned to: None
Originator Name: Open/Closed: Open
Release: devOperating System: GNU/Linux

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Mon 16 Apr 2018 06:37:00 PM UTC, comment #21:

I agree that this needs a full re-think and re-architecting. When that task is tackled we should look to use either Qt-provided or C++11 facilities for the actual operations (like mutexes, etc.) rather than writing our own.

Rik <rik5>
Project Administrator
Sat 14 Apr 2018 05:00:39 PM UTC, comment #20:

I agree Pantxo, some fundamental redesign along the lines of hand-shaking of some sort is needed. That's not a minor project--or at least not one that can be done in a week before a release. Seems like a good summer project, and it requires coordination between people who know both sides of the threads.

As you mentioned, in this case the asynchronous action is necessary whereas in other instances it may not be. Another example of the forced-synchronous approach is what I did some time ago with the inputdlg() family of routines. I forced synchronous timing between interpreter (T2) and GUI (T1) in a really crude way by putting T2 to sleep while the user is entering data in the inputdlg. Once the user closes the modal dialog, T1 will put the data in the desired location then wake up T2. So, basically T2 can't do anything when the inputdlgs are busy. Crude, but it ensures no thread problems. There wasn't much alternative at the time because no design was in place for communication between the components. So going forward, if ever Octave GUI-building (i.e., one can build their own GUI within Octave...not the "Octave GUI" we commonly refer to) the inputdlg approach I described is a no-go. There has to be active threads on both sides of the divide for something like that. Just something to keep in mind.

Dan Sebald <sebald>
Sat 14 Apr 2018 11:38:27 AM UTC, comment #19:

I'll try to sketch the way Octave interpreter thread (T2 hereafter) interacts with the Qt's GUI thread (T1 hereafter).

When a figure (or an ui* object) needs to be initialized, updated or finalized, the ObjectProxy (in T2) sends a signal. Depending on the context, we connect the signal to ObjectFactory::createObject, in case of initialization, or to the corresponding Object::update/finalize slots (all in T1). Since signals and slots live in two different threads, they will by default be treated asynchronously in two different Qt event queues.

The first consequence is that all ObjectProxy::initialize/update/finalize methods (in T2) will generally return before the operations in T1 have been successfully done. This bug and bug #53644 are illustrations of situations where we can't live with this asynchronous behavior.

For printing (and getting pixels) we have solved the above issue by forcing the signaling methods and corresponding slots to run synchronously. Basically we use a Qt::BlockingQueuedConnection which locks the emitting thread (T2) until the slot have been processed in T1.

Unfortunately we cannot apply this method to ObjectProxy::initialize/update/finalize since the corresponding slots rely on the interpreter thread T2 being awake:

  • we need to be able to change some values in the graphics_object properties (instanciated from T2). This is either done directly (we lock a mutex and call graphics_objet::properties::set_xxx) or we post a graphics event which will be processed later in T2.
  • we need to be able to ask the interpreter to run graphics callbacks. This is done by posting a graphics event.

For the sake of completeness, the graphics event queue is implemented using readline and is supposed to be flushed only when "drawnow" (and a few other functions) is requested.

To summarize, I think the question is "how do we make sure signaling methods in ObjectProxy don't return before their corresponding slots actually do?". ATM I only can say what we cannot do: lock T2 using a mutex or a "while (condition_not_met) {}".

Pantxo Diribarne <pantxo>
Project Member
Sat 14 Apr 2018 06:11:01 AM UTC, comment #18:

Have a look at this ASAN analysis

https://savannah.gnu.org/bugs/?53644

I think that bug is also a timing issue.

Dan Sebald <sebald>
Wed 11 Apr 2018 05:34:46 PM UTC, comment #17:

I put a conditional statement in bold because I don't know how this Qt toolkit and the multithreading is designed (nor do I care to know at this point). Member functions can be executed from one thread or another (the restriction being that graphics can only be done in Qt's main thread), but if you are saying Figure code is only executed from one thread, then that is fine. The point still holds about semaphores, though. Even in the graphics thread, using Qt's signal/slots rather than blocking the execution of code might be better.

From Canvas.cc:

From ObjectProxy.cc:

Then there are callback methods:

which I presume are coming from the worker thread, not the main thread.

The Figure::updateFigureToolBarAndMenuBar() code I listed below: Can you see how difficult it is to tell that "m_blockUpdates = true;" is meant to stop some recursion? Where is the "if (m_blockUpdates) return;"? It's in some other function. One has to infer there is a possible recursion. The alternative interpretation is that there is something asynchronous occurring that might do an update if that m_blockUpdates is not set.

Folks want to ditch other toolkits, but Qt toolkit hasn't become robust over the course of half a decade and several release versions. These little added delays, multiple tries, etc. simply are wasting effort in my opinion.

Dan Sebald <sebald>
Wed 11 Apr 2018 07:51:58 AM UTC, comment #16:

@Dan: I don't understand. m_blockUpdate is a private member variable and is thus only accessible from the Object itself (a Figure in your example). In this regard I don't see how the scenarios you describe could ever happen and how there could be any asynchronicity here (recursion at worst, and it is what m_blockUpdates is supposed to be preventing).

Pantxo Diribarne <pantxo>
Project Member
Tue 10 Apr 2018 04:15:13 PM UTC, comment #15:

That makes sense. Eventually we need to work out a good architectural solution to all of the cross-thread work.

One theoretical solution would be some sort of semaphor so that each side could be certain the other side had completed all of its actions.

Just for testing purposes, what happens if you put in a call to sleep somewhere in the delete() function stack so that the GUI or the CLI has a chance to complete all of its operations? Does that also fix bug #53604?

Rik <rik5>
Project Administrator
Tue 10 Apr 2018 04:10:00 PM UTC, comment #14:

What Pantxo is describing in Comment #13 and Rik in Comment #8 is what I meant by Comment #4. As an example, I look at the code and see pretty extensive use of the variable "m_blockUpdates" throughout. That variable is used in Figure::update (int pId) as follows:

and I see a process ID as an input, so I'm wondering (rhetorically) if this routine is asynchronous. If that is the case, that m_blockUpdates used all about won't do much. The m_blockUpdates variable approach will prevent recursive calls, but not asynchronous issues. For example, hypothetically speaking, imagine if this short routine is called asynchronously:

and some other thread is presently doing some sort of update for which it has already set m_blockUpdates to true. Well, in theory this routine could set m_blockUpdates "true", finish quickly and then a few machine cycles later set m_blockUpdates to "false" while the other thread is still working and has not yet set m_blockUpdates to "false". So m_blockUpdates would be reset prematurely and there is a little window of time where m_blockUpdates is "false" when it really should be "true". This is why I mentioned "max load". When the CPU is chugging away, a "short window of time" becomes a "big window of time".

Sometimes one can get away with this approach if there is only, say, one location where m_blockUpdates is set to "false" while maybe multiple places the variable can be set "true". Or use a MUTEX to hold off executing these routines until the data is free to be modified without consequence. Years ago I had talked about "comm link" where I prototyped communication between core/GUI at one finely controlled location--carefully analyzed reference count modifications and so on to test. That's a lot of work, and spreading this throughout the code seems even more difficult. I've only glanced at this code, so I don't know what the asynchronous issues are, if any, but I'm just saying if this is meant to address asynchronous issues, a redesign might be due.

Dan Sebald <sebald>
Tue 10 Apr 2018 02:47:43 PM UTC, comment #13:

I think I identified the issue. Before the following cset, the test used to pass

http://hg.savannah.gnu.org/hgweb/octave/rev/b6aea95a7bf9

The problem is that we rely on ref counting for graphics_object deletion (and further cleanup). Before the above cset, the toolkit Object, which lives in the GUI thread, did not have a reference to its grapĥics_object counterpart and gh_manager::free took care of deleting the last reference to the graphics_object. Now the asynchronous nature of the "graphics_toolkit::finalize" method makes the object eventually be deleted after "delete" has returned, hence the cleanup (remove the uicontextmenu from its dependent objects) not being already done when delete returns.

I don't have a solution at the moment but we now have an interpretation.

Pantxo Diribarne <pantxo>
Project Member
Wed 04 Apr 2018 04:26:21 PM UTC, comment #12:

Marking bug as Postponed. The delay works to have the test suite pass for the 4.4 release. But we should really determine the root cause of the problem.

Rik <rik5>
Project Administrator
Tue 03 Apr 2018 12:36:52 AM UTC, comment #11:

Yes. it works on my desktop as well.
Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Tue 03 Apr 2018 12:22:38 AM UTC, comment #10:

Okay, this isn't the best but I put in a 5ms delay after the delete() call. The buildbots seem much happier now. Does it work on your home system now?

Rik <rik5>
Project Administrator
Mon 02 Apr 2018 04:42:12 PM UTC, comment #9:

I tried to repeat the test on command line 10 times and I got 10 failures...

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Mon 02 Apr 2018 04:38:29 PM UTC, comment #8:

There is something intricate going on. I see that the buildbots are failing, but I can run 'test graphics.cc' in a loop without failure. However, if I run 'make check' from a command line I do get an overall failure.

Rik <rik5>
Project Administrator
Mon 02 Apr 2018 04:36:41 PM UTC, comment #7:

My workstation 34a5a420208e (stable) tip as well as buildbots
(dev) are still failing...

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Mon 02 Apr 2018 04:19:10 PM UTC, comment #6:

I just fixed another issue related to lint checking which appears to have resolved this bug for me.

I ran the test script in comment #3 50 times with no failures.

Marking bug as Ready for Test.

Rik <rik5>
Project Administrator
Fri 30 Mar 2018 10:06:52 PM UTC, comment #5:

Agreed. The delays are not a solution for the root problem. Typically, I think, we've had issues cross-thread issues between the GUI and Octave thread. But I don't think that is the case here. This looks related to the delete procedure which we should be able to make deterministic.

Rik <rik5>
Project Administrator
Fri 30 Mar 2018 09:52:33 PM UTC, comment #4:

Adding arbitrary delays like this or like

http://hg.savannah.gnu.org/hgweb/octave/rev/6bc4958b224b

are merely "papering-over" solutions to timing issues. I could give lengthy details of specific examples, but suffice it to say one is never sure what happens when a system is at max load or whether re-pinging an operation is going to affect data integrity of the process one is waiting on to complete. Unfortunately, it turns a bug into something rare and obscure but never total confidence something won't fail.

Dan Sebald <sebald>
Fri 30 Mar 2018 09:24:22 PM UTC, comment #3:

A little debugging shows that I do not have this problem, even in the GUI, if I am using the FLTK toolkit. This is something unique to the Qt toolkit. I extracted the one failing test in to a new file tst_ui.m which is attached. You can use 'test tst_ui' to run just this one problem test.

At least part of the issue is related to the new QTOFFSCREEN rendering that we are using for invisible figures. If I change the first line of the file to make the figure visible then 75% of the time the test passes. That still sucks, but it is an indication that whatever it is gets far worse when the figure is hidden. Just guessing, but when the figure is hidden graphics operations take place much quicker which might be showing what race condition exists.

Finally, it only takes one very small delay, I used a millisecond, after the call to delete to get this to work.

The assert which fires is saying that the uicontextmenu entry for the figure is not empty, as it should be after the delete, but still contains a 1x1 entry (presumably this is hctx2).

This leads me to believe that there is something amiss with the way we implement delete. The handle that we are deleting, hctx2, is destroyed immediately. But this will cause other callbacks to fire which then remove hctx2 from the "children" property of the parent of hctx2. I think control must be returning to the command line before those updates have had a chance to complete.

How is the event queue flushed? Is only one cycle done each time Octave returns to the command prompt? Or is the event queue fully emptied before returning to the command prompt. The second choice seems better because operations, like delete, could spawn new events which need to be processed.

(file #43733)

Rik <rik5>
Project Administrator
Fri 30 Mar 2018 07:32:28 PM UTC, comment #2:

I wonder if we have an issue with the way the schema database for graphics properties is implemented. Maybe updates are not atomic?

Rik <rik5>
Project Administrator
Thu 29 Mar 2018 09:03:36 PM UTC, comment #1:

Similarly to "getframe" issue, if I add pause(0.1) after each
set() and delete() then I do not get any failures.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Thu 29 Mar 2018 05:00:14 PM UTC, original submission:

With
d298a0734d85 (stable)

(also with dev -- see buildbots)

octave:1> test libinterp/corefcn/graphics.cc-tst
warning: axis: omitting non-positive data in log plot
warning: Non-negative limit for logarithmic axis ignored
warning: Non-positive limit for logarithmic axis ignored
warning: axis: omitting non-positive data in log plot
warning: Non-positive limit for logarithmic axis ignored

          • test

hf = figure ("visible", "off");
hax = axes ("parent", hf);
unwind_protect
hctx1 = uicontextmenu ("parent", hf);
hctx2 = uicontextmenu ("parent", hf);
set (hf, "uicontextmenu", hctx2);
set (hax, "uicontextmenu", hctx2);
assert (get (hf, "uicontextmenu"), hctx2);
assert (get (hax, "uicontextmenu"), hctx2);
assert (get (hf, "children"), [hctx2; hctx1; hax]);
delete (hctx2);
assert (get (hf, "uicontextmenu"), []);
assert (get (hax, "uicontextmenu"), []);
assert (get (hf, "children"), [hctx1; hax]);
set (hf, "uicontextmenu", hctx1);
assert (get (hf, "uicontextmenu"), hctx1);
set (hf, "uicontextmenu", []);
assert (get (hf, "uicontextmenu"), []);
assert (get (hf, "children"), [hctx1; hax]);
unwind_protect_cleanup
close (hf);
end_unwind_protect;
!!!!! test failed
ASSERT errors for: assert (get (hf, "uicontextmenu"),[])

Location | Observed | Expected | Reason
. O(1x1) E(0x0) Dimensions don't match

I get it with ~100% reproducibility.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #43733:  tst_ui.m added by rik5 (909B - text/x-matlab)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -unavailable- added by pantxo (Posted a comment)
  • -unavailable- added by sebald (Posted a comment)
  • -unavailable- added by rik5
  • -unavailable- added by rik5
  • -unavailable- added by rik5
  • -unavailable- added by rik5 (Updated the item)
  • -unavailable- added by dasergatskov (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 8 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 10 Apr 2018 04:15:21 PM UTCrik5Dependencies-=>bugs #53604 is dependent
    Wed 04 Apr 2018 04:26:21 PM UTCrik5StatusReady For Test=>Postponed
    Mon 02 Apr 2018 04:19:10 PM UTCrik5StatusConfirmed=>Ready For Test
    Fri 30 Mar 2018 09:24:22 PM UTCrik5Attached File-=>Added tst_ui.m, #43733
      Carbon-Copy-=>Added pantxo
      Carbon-Copy-=>Added sebald
    Fri 30 Mar 2018 07:32:28 PM UTCrik5Carbon-Copy-=>Added jwe
    Thu 29 Mar 2018 05:06:57 PM UTCrik5StatusNone=>Confirmed

    Back to the top


    Powered by Savane 3.1-cleanup1