Cataclysm-dda: use of flock() on map files is ineffective

[mutability]

(Properly documenting an issue mentioned in passing when discussing persistent IDs elsewhere)

There are a number of problems with how flock() is used (via mapsharing.cpp):

On write, flock(LOCK_EX | LOCK_NB) is used; this provides mutual exclusion between concurrent writers, but in the case of a concurrent write one of the writers will fail to save entirely rather than queuing up for the lock. Probably it should either drop the LOCK_NB, or retry a few times at intervals on failure. This becomes more important once the readers do correct locking, as a concurrent (shared lock) reader will prevent the exclusive lock from being granted.

There is no corresponding locking on read, so nothing prevents a reader from seeing a partial write (and so failing to load that submap and falling back to regenerating it). Readers should presumably lock with LOCK_SH.

On anything that isn't Linux, there is no locking at all.

[Coolthulhu]

The whole mapsharing feature is pretty broken and the only reason it isn't obvious is because no one uses it.
We should drop it rather than pretending we support it.

[kevingranade]

On Jan 22, 2017 8:40 AM, "Coolthulhu" [e-mail] wrote:

The whole mapsharing feature is pretty broken and the only reason it isn't
obvious is because no one uses it.

You have evidence of this, or are you assuming the mentioned bugs are fatal?

We should drop it rather than pretending we support it.

"Feature has some bugs" is extremely different from, "this feature is so
broken we should simply remove it". I haven't seen a case made for why map
sharing falls under the latter statement.

[Coolthulhu]

You have evidence of this, or are you assuming the mentioned bugs are fatal?

They aren't fatal as in causing crashes, but they're fatal as in they make the whole thing pointless and unstable.

There were a bunch of experiments with mapsharing and they all ended up with whole map griefed to hell and all spawn points covered in flames or burnt to the ground.
There are synchronicity issues with NPC IDs - all the potential issues with item and creature IDs already exist with NPC and mission IDs.
Moving back and forth in time means all rottable items are rotten, solar vehicle and funnel calculations are broken, zombies are maximally evolved and so on. This will only get more broken as the world gets more "alive".
Reality bubble limiting activity of flames - a problem that can't be easily solved - means it's easy to make the whole thing lag pretty hard for anyone with less than a modern gaming PC.

Mapsharing is simply incompatible with the idea of continuity. It only works in an immutable world.
If something is to be salvaged from it, it would be something like bones sharing - having dead characters and a bunch of surrounding zombies spawn in someone's world or something like that.

"Feature has some bugs" is extremely different from, "this feature is so broken we should simply remove it".

Mapsharing is a barely supported feature no one uses, which prevents much more important features such as item ID from being properly considered (despite having the same problems as item ID already in). At this point it's clearly more of a burden than if it was so broken it crashed on start.

It's as if "Defense" game mode locked us out of z-levels.

[kevingranade]

On Mon, Jan 23, 2017 at 6:22 AM, Coolthulhu [e-mail]
wrote:

You have evidence of this, or are you assuming the mentioned bugs are
fatal?

They aren't fatal as in causing crashes, but they're fatal as in they make
the whole thing pointless and unstable.

There were a bunch of experiments with mapsharing and they all ended up
with whole map griefed to hell and all spawn points covered in flames or
burnt to the ground.

1. In other words, you have in fact heard of people using it.
2. Spawn point exhaustion doesn't strike me as a particularly difficult
   peoplem, and it's one shared by single player game modes as well.

There are synchronicity issues with NPC IDs - all the potential issues
with item and creature IDs already exist with NPC and mission IDs.
Moving back and forth in time means all rottable items are rotten, solar
vehicle and funnel calculations are broken, zombies are maximally evolved
and so on. This will only get more broken as the world gets more "alive".
Reality bubble limiting activity of flames - a problem that can't be
easily solved - means it's easy to make the whole thing lag pretty hard for
anyone with less than a modern gaming PC.

Mapsharing is simply incompatible with the idea of continuity. It only
works in an immutable world.

Literally every one of these issues except running multiple processes
concurrently causing high resource usage also can happen with consecutive
or alternating games in the same world. Not everyone considers this a
major problem, ultimate consistency really isn't that big a deal in a large
enough game world. For people that do care, they can simply play one
player to a game world.

"Feature has some bugs" is extremely different from, "this feature is so
broken we should simply remove it".

Mapsharing is a barely supported feature no one uses, which prevents much
more important features such as item ID from being properly considered
(despite having the same problems as item ID already in). At this point
it's clearly more of a burden than if it was so broken it crashed on start.

This is utterly ludicrous, I outlined a simple and workble system for this
that meets everyone's requirements months ago. If the very modest
additions required to make this work with multiple game instances make it
too difficult for someone to imlement, I don't want them mucking around
with game code that fundamental in the first place.

[mutability]

I outlined a simple and workble system for this that meets everyone's requirements months ago.

Not true; it met _your_ requirements but it was no good for the things that I was interested in doing ID work for (notably, any long lived relationship between objects that could span the reality bubble) and you made it clear that you would not accept an alternative, so I stopped working on it.

Anyway, that's not what I created this issue to argue about, I created the issue so that you were aware of one more small problem with mapsharing and could fix it if mapsharing bugs are important.