Maproom / qmapshack

Consumer grade GIS software
GNU General Public License v3.0
271 stars 63 forks source link

[QMS-624] Suspicion: QMS crashes if BRouter RD5 file is missing #633

Closed ntruchsess closed 2 months ago

ntruchsess commented 10 months ago

What is the linked issue for this pull request:

QMS-#624

What you have done:

fixed potential race on CRouterBRouter field 'synchronous'. (It is not propagated to the request-finished slot as a reply property). removed use of mutex as the field 'synchronous' that was protected by the mutex no longer exists

Steps to perform a simple smoke test:

BRouter (offline) on-the-fly routing and regular routing should work without crash or freeze.

Does the code comply to the coding rules and naming conventions Coding Guidelines:

Is every user facing string in a tr() macro?

Did you add the ticket number and title into the changelog? Keep the numeric order in each release block.

wthaem commented 10 months ago

I'm a bit confused - there are too many PR for me.

What I did:

Result: Crash as before.

ntruchsess commented 10 months ago

I'm a bit confused - there are too many PR for me.

this PR is based on kiozen QMS-624, to test there is no need to check out anything else.

wthaem commented 10 months ago

What I did:

git fetch ntruchsess
git checkout -b ntruchsess1 ntruchsess/QMS-624
Switched to a new branch 'ntruchsess1'
branch 'ntruchsess1' set up to track 'ntruchsess/QMS-624'.

git log -3
commit e0399d31a69433b96a36f9ed0de94802219d8327 (HEAD -> ntruchsess1, ntruchsess/QMS-624)
Author: Norbert Truchsess <norbert.truchsess@t-online.de>
Date:   Fri Aug 11 21:50:48 2023 +0200

    QMS-624 fix BRouter mutex threading issues

...

After that I built QMS as RelWithDebInfo and applied the steps described earlier. Result: Crash. The crash does not yet happen as long as the mouse is still moving. A little while after the mouse comes to a standstill the mouse cursor changes to a rotating circle and again after a little while QMS crashes.

Then I used MSVC for stepwise debugging. Result: With one of the calls to update() in CCanvas::slotTriggerCompleteUpdate line 804 some Qt modules are called and the crash happens. Before this call is some message and mouse handling.

ntruchsess commented 10 months ago

After that I built QMS as RelWithDebInfo and applied the steps described earlier. Result: Crash. The crash does not yet happen as long as the mouse is still moving. A little while after the mouse comes to a standstill the mouse cursor changes to a rotating circle and again after a little while QMS crashes.

Then I used MSVC for stepwise debugging. Result: With one of the calls to update() in CCanvas::slotTriggerCompleteUpdate line 804 some Qt modules are called and the crash happens. Before this call is some message and mouse handling.

That's an interesting result. I removed the calls to slotTriggerCompleteUpdate() https://github.com/Maproom/qmapshack/pull/633/commits/a35aba285d7cf5f4570eea0b9a020bb2b1958a2f . The caller would update the canvas anyway when receiving the routing-result. (Those calls were copy&paste from the Routino and Mapquest-routers, I never evaluated whether they are actually required. Turns out they are not).

please update and re-check to see whether it makes a difference.

@kiozen : I can see there are quite a lot of direct calls to slotTriggerCompleteUpdate() in the code-base. From my understanding this is only save when all related code is guaranteed to run on the same thread. I suggest that should be changed to utilize Qt-signalling wherever it's unclear that this constraint is fulfilled.

wthaem commented 10 months ago

Sorry, no change. Still crashing.

ntruchsess commented 10 months ago

Sorry, no change. Still crashing.

can you please post the call-stack? As relevant parts in the router have changed the resulting call-stack has certainly changed as well and I cannot proceed without.

kiozen commented 10 months ago

And use a debug version not relwithdebuginfo.

wthaem commented 10 months ago

The call stack is similar or equal to the last one shown in the discussion of issue #624. With the exception of qmapshack.exe!main(int argc, char * * argv) Zeile 75 all files mentioned are Qt files.

I'll try a debug version in the next few days.

wthaem commented 10 months ago

I wasn't aware anymore that building a debug version implies rebuilding all DLLs used in QMS as debug versions. This will take some time.

Building a release with debug info requires only rebuilding the qmapshack.exe - much faster.