rock-simulation / mars

MARS is a cross-platform simulation and visualisation tool created for robotics research.
https://github.com/rock-simulation/mars/wiki
62 stars 42 forks source link

fixed signal handling (fixes shutdown by crtl-C) #42

Closed planthaber closed 9 years ago

malter commented 9 years ago

Hey, I'm going to revert this pull request with the next merge from develop to master because I doubt that this is a correct bugfix, it just prevents the segfault (which I couldn't reproduce) by closing the simulation without cleaning it.

planthaber commented 9 years ago

And preventing a segfault is something you shouldn't do and not a bugfix?

The segfault occurs when qt is closed by signal, before quit() was called on the Base class, not on the child class (MyApp).

I don't see why the simulation is not cleaned? All this change dies is calling quit() on the MyApp class instead using QApplication->quit() (calles via the global qApp pointer) .

I can't see why mars does not execute the cleanup. And if it does, it also never did a cleanup on exiting based on a signal

goldhoorn commented 9 years ago

Whereever the problem is, this PR is not wrong at all why reverting it? It maybe double-checks, this is not a bad behaviour at all

malter commented 9 years ago

With the commit quit() is never called because the myApp pointer is never set (is always NULL).

As I said before I was not able to reproduce the segfault. I did some cleaning of the general exit behavior on the develop branch. So with the next merge from develop to master there shouldn't be a segfault and the simulation should exit normally with cleaning up the libraries.

planthaber commented 9 years ago

Ah, indeed the myApp pointer is not set, when needQApp is false. But why should it? when there is a QApplication from"another code part" , "another code part" should take care of calling quit()

malter commented 9 years ago

There was a bug even before your pull-request if got it right. The app pointer used in app->exec() was never set because a local new variable was set in the if statement. The application should have crashed all night long. ;-) I don't know why we can call the QApplication->exec() method on a NULL pointer. Your pull request reorganized the app pointer around that local variable but didn't touch the real problem.

planthaber commented 9 years ago

The application should have crashed all night long. ;-) I don't know why we can call the QApplication->exec() method on a NULL pointer.

"qApp" is a global variabe set by Qt itself every time the QApplication constructor is called. So the local myapp was deleted and exec was called on the parent QApplication (through the qApp pointer) istead of using the myApp Pointer, which was indeed local.

This is very confusing, and this is also te reason why this PR should stay in the code

planthaber commented 9 years ago

sorted out, malte created a similar one, actually quit() was never called (preventing the segfault)