Skip to content

Serialise BlockSchematicStructure.Unpack's lazy initialisation - #53

Open
Zaldaryon wants to merge 1 commit into
anegostudios:masterfrom
Zaldaryon:fix/blockschematicstructure-unpack-race
Open

Serialise BlockSchematicStructure.Unpack's lazy initialisation#53
Zaldaryon wants to merge 1 commit into
anegostudios:masterfrom
Zaldaryon:fix/blockschematicstructure-unpack-race

Conversation

@Zaldaryon

Copy link
Copy Markdown

Unpack() initialises the schematic lazily on first use, with an unsynchronised check-then-act:

public void Unpack(ICoreAPI api)
{
    if (blocksByPos == null)
    {
        Init(api.World.BlockAccessor);
        LoadMetaInformationAndValidate(api.World.BlockAccessor, api.World, FromFile);
    }
}

A BlockSchematicStructure instance is shared by every worldgen thread that places that structure, so with MaxWorldgenThreads above 1 two threads can both see blocksByPos null and both run the initialisation.

The more damaging case is quieter. Init assigns blocksByPos partway through its own work, well before LoadMetaInformationAndValidate has run. So a second thread can find blocksByPos non-null, return from Unpack believing the schematic is ready, and then read fields that nothing has populated yet. PathwayStarts is one of those, and reading it early throws.

This puts both entry points behind one per-instance lock.

There is no lock-free fast path on purpose. blocksByPos being non-null does not mean unpacking has finished, so a caller cannot use it to decide the lock is unnecessary. Since the lock is per instance and only contended on the first placements of a given schematic, the cost after warm-up is an uncontended monitor acquire.

Found while running worldgen with several threads. Related but separate: anegostudios/VintageStory-Issues#9871.

Unpack() lazily initialises the schematic on first use, with an unsynchronised
check-then-act. The same BlockSchematicStructure instance is shared by every
worldgen thread that places that structure, so with MaxWorldgenThreads above 1
two threads can both see blocksByPos null and both run the initialisation.

The worse case is subtler. Init assigns blocksByPos partway through its own work,
before LoadMetaInformationAndValidate has run, so a second thread can find
blocksByPos non-null, return from Unpack believing the schematic is ready, and
read fields that are not populated yet. PathwayStarts is one of those, and
reading it early throws.

Both entry points now take the same per-instance lock. There is deliberately no
lock-free fast path, because blocksByPos being non-null does not mean unpacking
has finished, so a caller cannot safely skip the lock on that basis.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant