News:

Do you need help?
Simutrans Wiki Manual can help you to play and extend Simutrans. In 9 languages.

Crash on rotation

Started by Jando, April 25, 2019, 09:14:10 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

Jando

I have this problem as well, rotating my current map crashes the game. Interestingly, if you rotate the map while the game is paused it does not crash at once but only after unpausing the game after rotation.

Vladki

I have similar problem, crash is soon after rotation. I thought it was due to pak128.cs, but will try with other paksets.

Ranran

#2
This seems to be the cause different from my first post.
Now the camera rotation will definitely cause a crash.
It also crashes with pak128.Britain-Ex. Load demo.sve and rotate the camera.

EDIT:
I used a recent release build and checked with demo.sve if camera rotation causes a crash.
All builds since 2019/04/07 caused a crash.
It does not crash on 2019/03/23 builds.

TurfIt

freelist has mutex protection; It has been called multithreaded for the map update on loading and rotating for a while - years.

To me:   FIX: More robust and consistent handling of building tiles, commit 6a620645671cfda93fe091120ab6fd774be9a710 looks suspicious.

in gebaeude_t::rotate90(), searching adjacent tiles while they are being modified by other threads?? ??

ACarlotti

#4
Quote from: TurfIt on April 25, 2019, 07:59:06 PMTo me:   FIX: More robust and consistent handling of building tiles, commit 6a620645671cfda93fe091120ab6fd774be9a710 looks suspicious.
I can produce a segfault with commit 6a6206, but not (in this manner) with 6a6206~ (i.e. the parent commit). The segfault occurs while running generate_passengers_or_mail, when it encounters a tile whose halt_list pointer is null.
It is not a threading issue - the segfault also occurs (at the same point) with a single threaded build.

Edit: So gebaeude_t::rotate90() is called during the running of karte_t::rotate90_plans, which means that at the time it is called, welt->lookup will be returning tiles from the old plan array (which may have been swapped with dummy tiles), while the building has already switched to the new coordinates. So clearly this will cause problems.

I think this comment someones thoughts on encountering a similar issue for factories:

// Factories need their halt lists recalculated after the halts are rotated.  Yuck!
FOR(vector_tpl<fabrik_t*>, const f, fab_list) {
f->recalc_nearby_halts();
}

jamespetts

Thank you for your reports and investigations in this matter. I did not realise that the rotation was multi-threaded, leading to some code that was not thread-safe. I have now fixed this, which fix will be in the next nightly build.

I should be grateful if anyone could confirm with that next build whether this has been fixed.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

ACarlotti

Quote from: jamespetts on April 26, 2019, 10:01:28 PMI did not realise that the rotation was multi-threaded, leading to some code that was not thread-safe.
You've misunderstood the problem slightly - the problem is not an issue of thread-safety, but rather that you were trying to elements of an array while that array was being modified in preparation for replacing it with a different away. There were potentially some race conditions involved, but the fundamental issue is not threading related. Indeed, the world_xy_loop code has some signalling to (I believe) prevent it working on nearby tiles simultaneously, which would probably be sufficient to deal with thread-safety issues in cases like this, were the coordinates not in the process of being changed due to rotation.

jamespetts

Quote from: ACarlotti on April 26, 2019, 11:37:53 PM
You've misunderstood the problem slightly - the problem is not an issue of thread-safety, but rather that you were trying to elements of an array while that array was being modified in preparation for replacing it with a different away. There were potentially some race conditions involved, but the fundamental issue is not threading related. Indeed, the world_xy_loop code has some signalling to (I believe) prevent it working on nearby tiles simultaneously, which would probably be sufficient to deal with thread-safety issues in cases like this, were the coordinates not in the process of being changed due to rotation.

Thank you for the explanation; is there any problem that you see with the fix to the issue, or is this at least correct?
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

ACarlotti

I haven't looked at the fix in detail - I'm confident that it fixes the original bug, but I don't know whether the new code has any bugs of its own.

Vladki

Now I have no crash after rotation.

jamespetts

Splendid, thank you both for confirming.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Ranran

This bug has not been resolved yet. :warning:
The new bug that was born on 4/7 has just been fixed.
The saved game I posted at the beginning of this thread still causes a crash.  ::'(

jamespetts

I have split this from the topic regarding the other rotation issue, which is much harder to solve and much rarer.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Ranran

I think that prissi mentions here about unsettled bug.

jamespetts

Quote from: Ranran on April 27, 2019, 01:46:56 PM
I think that prissi mentions here about unsettled bug.

Where do you mean by "here"?
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Ranran

In this thread:
Quote from: prissi on April 25, 2019, 02:56:15 PMThis error message means either that you are freeing something that was already freed or rather that something was freed which was overwritten with something larger. Freelist is just a faster way to alloc lots of chunk of identical memory size (i.e. trees). It saves a lot memory and is about 3x faster than malloc/free (or new/delete without freelist).
Quote from: DrSuperGood on April 25, 2019, 03:32:51 PMIs the freelist thread safe for both allocations and frees? One of the reasons malloc/new is usually so slow is thread safety.

I think this is content for another thread.

jamespetts

Thank you for letting me know - I have now put these messages back in the other topic.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Vladki

There are still some crashes related to rotation. I'm experiencing them often, but can't find what is causing them, not even a reliable reproduction case. So far it happens mainly if you rotate several times in quick succession

jamespetts

I will not be able to diagnose this without a reliable reproduction case, I am afraid. If you are able to produce one, I should be grateful if you could let me know.

Thank you.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.