bugGNU Octave - Bugs: bug #50025, Octave window freezes when I quit...

 
 

bug #50025: Octave window freezes when I quit Octave GUI

Submitted by:  Daniel <trez>
Submitted on:  Wed 11 Jan 2017 02:08:30 PM UTC  
 
Category: GUISeverity: 3 - Normal
Priority: 5 - NormalItem Group: Segfault, Bus Error, etc.
Status: Need InfoAssigned to: None
Originator Name: Open/Closed: Open
Release: 4.2.0Operating System: Mac OS

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 15 Jan 2018 12:16:02 PM UTC, comment #34:

@Dan

Unfortunately, the fprintf(stderr, "\n") is still needed.

Ben Abbott <bpabbott>
Project Member
Mon 15 Jan 2018 06:57:41 AM UTC, comment #33:

You may be on to something, but I'm hoping you meant that this patch works without the need of the fprintf() call, i.e., no

If I remove the above line from the patch you posted, Octave GUI still exits fine in linux--no surprise, I guess.

I probably disregarded this connection:

thinking it just some innocuous extraneous call at worst. However, that might have been the source of the problem all along. What this connection means is once the main thread finishes, it's going to emit a signal that the main thread (itself) should delete later. That does seem like an odd construct, though, doesn't it? I mean, this might be an unconventional thing to do, and as such the Qt developers may have a bug in this construct that went untested. It's conceivable that m_main_thread can't change the status of delete-later once it has reached the stage of issuing a finished().

...

In fact, I'm looking at the documentation for Qt here (it's the link from post #3):

http://doc.qt.io/qt-5/qthread.html#details

Look at the second method they suggest for doing this worker-thread communication. It's the worker thread that has its finished() signal connected to its deleteLater() slot:

and, importantly, the WorkerThread must be subclassed from QThread, i.e., derived from.

In Octave, it looks like the main thread is connected to itself (not the worker thread) and the main thread is just a QThread, not a derived class. The current configuration is probably some unconventional thing for which it might be no surprise (to the designers perhaps) that this fails.

Anyway, let me know if your revised patch still works if the fprintf() is removed. If not, we may have to redo this worker-thread setup as much as possible similar to the examples given in the link above. (Threads are tricky, not deviating is a good idea.)

Dan Sebald <sebald>
Mon 15 Jan 2018 04:30:51 AM UTC, comment #32:

@Dan, I'm just blindly experimenting and found the change below also allow Octave to exit normally. I don't have any insight, but maybe you ore someone else will.

Ben Abbott <bpabbott>
Project Member
Thu 11 Jan 2018 06:01:56 AM UTC, comment #31:

OK, let me think about this.

Dan Sebald <sebald>
Thu 11 Jan 2018 05:32:43 AM UTC, comment #30:

@Dan - Good call. Removing the wait also works.

I also expect that the fprintf() is fixing an existing problem, but have no idea how to track it down.

Ben Abbott <bpabbott>
Project Member
Thu 11 Jan 2018 05:20:47 AM UTC, comment #29:

I'm sure this still works because it was an

before and now an

However, this doesn't seem like a comfortable fix to the problem, i.e., placing an inexplicable fprintf() prior makes it work.

Perhaps the fprintf() is doing something like flushing a buffer somewhere as a side effect. What happens when removing the wait() and just have the fprintf() then quit()? Freeze or exit?

Dan Sebald <sebald>
Thu 11 Jan 2018 04:10:50 AM UTC, comment #28:

I'm now able to build with Qt5 using the default branch. The Qt patch ((file #41891) prevents the hang on exit, but the 1st debug fprintf(stderr,...) is needed to prevent the GUI from hanging on exit. I'm attaching a simpler version of the patch which reduces the fprintf content to \n.

At this point, I think we need a Linux user to test the patch to confirm that it doesn't break anything.

(file #42886)

Ben Abbott <bpabbott>
Project Member
Wed 10 Jan 2018 05:41:42 AM UTC, comment #27:

The Qt patch (file #41891) works for me with Qt4 using the default branch. However, if I remove the debug fprints, it does not work (i,e, it hangs on exit). The later patches also hang for me on exit.

Ben Abbott <bpabbott>
Project Member
Mon 20 Nov 2017 10:31:31 PM UTC, comment #26:

I think we could use the Qt::BlockingQueuedConnection if the signal were originating from the main window of the GUI. So perhaps what we need is something like

command_interpreter SIGNAL octave_finished_signal
goes to
main_window SLOT (initiate_quit???)

and the the main window has a connection

main_window, SIGNAL (quitmainthread???)
goes to
main_thread SLOT (quit ()) with Qt::BlockingQueuedConnection

That is, the main_window is the one that always quits the main_thread and then waits on it.

Dan Sebald <sebald>
Mon 20 Nov 2017 10:23:36 PM UTC, comment #25:

I may have placed the connection classification on the wrong signal/slot pair. Not sure. The problem is that it's now the interpreter issuing this signal (octave_finished_signal (int)), but the interpreter lives in the main_thread (via move) so I'm not sure if Qt::BlockingQueuedConnection even applies anymore because signal/slot in the same thread are not queued.

So, it may be that we have to go back to a setup similar to the original patch in which the quit/wait is done via the main window.

(file #42446)

Dan Sebald <sebald>
Mon 20 Nov 2017 10:03:05 PM UTC, comment #24:

It looks as though on the stable branch that JWE shuffled some code around and associated the m_main_thread with the octave_qt_link object. Hence m_main_thread is no longer accessible.

So, I've tried the idea from Panxto, which is to make the connection between finish() and deleteLater() blocking, which I'm hoping is essentially the same this as waiting. So, here's a patch to try for the stable branch. And perhaps if that works in stable it can easily be incorporated in default branch.

(file #42445)

Dan Sebald <sebald>
Mon 20 Nov 2017 09:34:25 PM UTC, comment #23:

jwe: I made a fresh checkout of the stable branch and compiled it (it still calls itself 4.2.1, right?). Octave still freezes on exit if one has plotted before.

Sebastian <sschoeps>
Mon 20 Nov 2017 08:53:15 PM UTC, comment #22:

Sebastian: I'd like to know what happens with the current stable branch sources without any patches applied.

John W. Eaton <jwe>
Project Administrator
Mon 20 Nov 2017 08:40:40 PM UTC, comment #21:

>Sebastian, we are wondering if the current development branch has the issue with
> freezing at exit in MacOS corrected or if there still needs to be some kind of patch.
> Would it be easy for you to test the most recent development branch that is going to
> form the basis for 4.2.2?


Sure. Will do. However, I assume Mike was rather sceptical that your patch can be backported (see comment #16).

Sebastian <sschoeps>
Mon 20 Nov 2017 08:31:52 PM UTC, comment #20:

Sebastian, we are wondering if the current development branch has the issue with freezing at exit in MacOS corrected or if there still needs to be some kind of patch. Would it be easy for you to test the most recent development branch that is going to form the basis for 4.2.2?

Dan Sebald <sebald>
Thu 12 Oct 2017 08:13:48 AM UTC, comment #19:

Oh yeah, you're right. I forgot that the signal type can be altered when making the Qt connection. The wait of quit/wait construct effectively creates a lock (I think, but haven't explored). Is there any difference between the two constructs? I'm not sure. Nonetheless, the quit/wait construct is an example as shown in the Qt documentation, so it's got that going for it.

I'm open to the blocking queued signal approach, though, but at the same time prefer not experimenting too much with something that seems to work, at least for now, in an otherwise historically fragile exit process. If/when the exit is overhauled to be more robust is a better time for experimenting.

Dan Sebald <sebald>
Mon 09 Oct 2017 01:52:45 PM UTC, comment #18:

@Dan:

>> The signal/slot connection doesn't have a means for waiting


I don't know if it is relevant here since since I did not look into this at all, but ther is a way to have the signal function return only when the slot has returned. The connection must be made in queud mode:
connect (signal,..., Qt::BlockingQueuedConnection)

There is a requirement though, the signaling thread is then locked until the slot thread has returned: it means that the slot thread can't request some operation to be done in the signal thread because it would lead to a dead lock.

Pantxo Diribarne <pantxo>
Project Member
Sat 07 Oct 2017 05:55:13 PM UTC, comment #17:

Correct, in all likelihood the QThread::wait is the necessary item. The signal/slot connection doesn't have a means for waiting.

Hopefully the next Octave release isn't too far off. There have been quite a few seemingly critical bugs following the most recent release, so it would be nice to get something more stable out there.

Dan Sebald <sebald>
Sat 07 Oct 2017 05:33:31 PM UTC, comment #16:

It's unlikely that this could be backported to the stable branch, but anyone is welcome to try. There has already been very significant refactoring work done in the GUI on the development branch before this point.

As far as applying this patch to the default branch, I see no problems with it. I suppose the relevant change here is the QThread::wait for completion before destroying the thread.

Mike Miller <mtmiller>
Project Administrator
Tue 03 Oct 2017 02:12:01 AM UTC, comment #15:

I can't make changes in the canonical repository. It's a very small changeset, so backporting doesn't seem complicated if you think it would help. (Most of the main developers don't work much with MacOS.) However, it would probably be good to make sure several MacOS testers feel comfortable the fix works before backporting.

Dan Sebald <sebald>
Mon 02 Oct 2017 07:15:54 PM UTC, comment #14:

I'd also be interested in seeing a backport. Last week, I ran into this issue in front of my physics class, with octave 4.2.1, built using MacPorts.

Marius Schamschula <mschamschula>
Sun 01 Oct 2017 06:13:20 PM UTC, comment #13:

@Dan: did you consider a backport?

Sebastian <sschoeps>
Sun 01 Oct 2017 05:20:31 PM UTC, comment #12:

OK, thanks for the clarification. I don't know the MacOS build process, but it sounds like it takes some work and finding another user to test might be difficult. It's up to Mike how to proceed.

Dan Sebald <sebald>
Sun 01 Oct 2017 05:03:32 PM UTC, comment #11:

>Or is it the case that you think you didn't have patch 41889 applied and built cleanly in the first place, but now it seems to work?


sorry for the confusion. I am pretty sure that my first attempt was not a clean build. Now, I have verified that both versions of your patch work for me and I always started from a fresh checkout :)

Sebastian <sschoeps>
Sun 01 Oct 2017 04:56:25 PM UTC, comment #10:

Mike is on this thread, so he'd be the one to incorporate the patch if it works. I'm a bit unclear about which patch works because you initially wrote something about it not solving the bug. Patch 41889 and 41891 are effectively the same, the latter having a little extra debug info. So if the latter works and the former doesn't, then that is simply a matter of the extra fprintfs slowing things down and not addressing the main problem. Or is it the case that you think you didn't have patch 41889 applied and built cleanly in the first place, but now it seems to work?

Dan Sebald <sebald>
Sun 01 Oct 2017 04:13:49 PM UTC, comment #9:

I checked on several machines with various qt and macOS versions. The patch works for me! I strongly suggest to push it to the development branch. I tried to back port it but I was not able. @Dan: would that be possible?

Sebastian <sschoeps>
Tue 26 Sep 2017 07:31:40 AM UTC, comment #8:

Well, thread timing bugs are some of the stranger bugs. When things aren't coded according to the design, changing things slightly in either thread can "fix" things or "break" them.

It's odd that you don't see any of the fprintf(stderr,...) output. If you are using "exit" at the command line to close the GUI, it would have to go through

main_window::handle_octave_finished

and hence at least the first fprintf() output should appear. I don't know about Mac OS X, though. Perhaps stderr is directed somewhere other than the terminal window by default.

Dan Sebald <sebald>
Tue 26 Sep 2017 07:08:55 AM UTC, comment #7:

I am very confused. I just compiled head with your patch. I see no output at all (with or without "--no-gui") but even more crazily: it does not freeze anymore.

I will try to find out what is going on later today.

Thanks,
Sebastian

P.S.: yes, I am sure that I am looking at the correct version of octave :)

Sebastian <sschoeps>
Mon 25 Sep 2017 03:31:53 PM UTC, comment #6:

We need to par down the amount of debug info to get an idea of what's causing the crash. I'm attaching the same patch, but I put some prints within the code that should be easy to recognized even if they are mixed in with some system trace. Could you run Octave from a command line and check if there is any meaningful output at exit? First try:

./run-octave --no-gui

and see if there is an error. (Even though the option says "no-gui", the GUI is actually launched, just not displayed.) And then run

./run-octave &

(file #41891)

Dan Sebald <sebald>
Mon 25 Sep 2017 08:42:11 AM UTC, comment #5:

Thanks Dan! I really appreciate this. The GUI is nicely working on macOS besides freezing on exit and some scaling issues of the plot engine on high-dpi screens. It seems rather minor stuff that stops mac users from using the GUI...

Sadly, your patch is not preventing that octave freezes. How can I provide helpful debug output?

Sebastian

Sebastian <sschoeps>
Sun 24 Sep 2017 09:09:21 PM UTC, comment #4:

Let's try a simple little change (attached patch). All this does is move the thread quit() to the GUI's main window which will allow also waiting on that quit. It works here on Linux. Let me know if it works on Mac OS X. That's about all I have time to explore right now.

(file #41889)

Dan Sebald <sebald>
Sun 24 Sep 2017 08:26:36 PM UTC, comment #3:

Folks, I think this is more of an Octave proper-Qt-programming issue than it is an OS X problem. In particular, it is the proper sequence of shutting down threads. I looked at the system complaints in the bug report

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

and although the dump is so voluminous to really not be helpful, I do see "pthread" and such in the list.

Let me try to illustrate what the issue probably is from looking at hunks of code.

The following is from mainwindow.cc:

What I wanted to explore is whether not starting the thread results in a clean thread. But when I comment out the m_main_thread->start() line on Linux, the GUI hangs and only kill -9 will stop it. Well, that's bad in the sense that Octave GUI should be much more graceful if the thread doesn't start properly. It doesn't seem far-fetched that a thread doesn't start, so here is another thing to look at. (My feeling is the whole startup/shutdown concept needs an overhaul.)

Now, shutdown. octave_finished_signal (probably emitted when the interpreter is about to exit) is connected to "this" (i.e., the GUI main window) handle_octave_finished(). That's fine, it's just letting the GUI know it's done.

The m_main_thread finished() signal is connected to its deleteLater() slot. This is fine too, as it has become a typical construct, at least in this prototypical example:

http://doc.qt.io/qt-5/qthread.html#details

Where I think the problem lies is this line:

The m_interpreter is running in the thread, and it tells the thread to quit. The Qt documentation makes no mention of such a construct, but I'm imagining that being an undefined sort of behavior. Let's take a look at the prototype example of how quit is used:

Notice it is the Controller, the thing that launched the worker thread, that is issuing the quit. The quit is not coming from within the thread. But more than that, and probably the key issue, is that the Controller can also wait on the thread to finish. If the GUI does not wait on the worker thread to finish, somewhere in the exit process, then there is a good chance this is problematic. I suspect that is the source of failure for Mac OS X, i.e., that threads are structured differently internal to the OS.

Notice with the signal/slot construct of accessing quit(), there is no straightforward way of also doing a wait() somewhere. This is why I'm saying the exit process needs to be reworked. Add to that the GUI should also gracefully handle the thread not starting properly. Also, the GUI should gracefully handle the interpreter crashing and the interpreter not responding at exit. There is just a whole lot of work that needs to be done.

Dan Sebald <sebald>
Fri 27 Jan 2017 07:37:16 AM UTC, comment #2:

Yes. Sadly, this is known. However, it might be that there was no bug report until now. This problem is there since we moved to qt5. I guess jpswensen is currently the only one who might be able to help here (see #49053).

Sebastian <sschoeps>
Fri 27 Jan 2017 01:48:42 AM UTC, comment #1:

Adding sschoeps in cc. As far as I know it is known that the GUI can crash and freeze several different ways on macOS. If you can help debug and help make Octave more stable, please do so.

Mike Miller <mtmiller>
Project Administrator
Wed 11 Jan 2017 02:08:30 PM UTC, original submission:

I have installed Octave version 4.2.0 via homebrew and created an Apple scriptfile with which I can start Octave Graphical User Interface.
But when I quit Octave GUI with simply closing it by clicking 'quit octave-gui' or typing 'quit' in command line, Octave shut down as it should but the window freezes and doesn't close until I force it to close with Apples Activity Manager.

Daniel <trez>

 

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

Attach File(s):
   
   
Comment:
   

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -unavailable- added by bpabbott (Posted a comment)
  • -unavailable- added by jwe (Posted a comment)
  • -unavailable- added by pantxo (Posted a comment)
  • -unavailable- added by mschamschula (Posted a comment)
  • -unavailable- added by sebald (Posted a comment)
  • -unavailable- added by sschoeps (Posted a comment)
  • -unavailable- added by mtmiller (Posted a comment)
  • -unavailable- added by mtmiller
  • -unavailable- added by trez (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 9 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 11 Jan 2018 04:10:50 AM UTCbpabbottAttached File-=>Added octave-qt_wait_on_worker_thread-djs2017sep25.patch, #42886
    Mon 20 Nov 2017 10:23:36 PM UTCsebaldAttached File-=>Added octave-qt_wait_on_thread-djs2017nov21.patch, #42446
    Mon 20 Nov 2017 10:03:05 PM UTCsebaldAttached File-=>Added octave-qt_wait_on_thread-djs2017nov20.patch, #42445
    Mon 25 Sep 2017 03:31:53 PM UTCsebaldAttached File-=>Added octave-qt_wait_on_worker_thread-DEBUG1-djs2017sep25.patch, #41891
    Sun 24 Sep 2017 09:09:21 PM UTCsebaldAttached File-=>Added octave-qt_wait_on_worker_thread-djs2017sep24.patch, #41889
    Mon 12 Jun 2017 06:16:44 PM UTCmtmillerDependencies-=>bugs #51216 is dependent
    Thu 27 Apr 2017 08:18:58 PM UTCmtmillerDependencies-=>bugs #50795 is dependent
    Fri 27 Jan 2017 01:48:42 AM UTCmtmillerStatusNone=>Need Info
      Carbon-Copy-=>Added sschoeps

    Back to the top


    Powered by Savane 3.1-cleanup1