[Feature Request] Better Level Management

[gentlegiantJGC]

The Problem

Currently a call to the load_level or load_format method in our core library returns a new instance of the level wrapper. This has currently been fine because each level tab owns the level associated with it. The issue happens when something separate to that level tab wants to access that world. An example of this if a level is opened in a level tab and also in the converter in another level tab.

Feature Description

We should have some sort of caching system that can return a previously opened instance rather than opening it again since this can cause issues. We would also need to add a system to release the level when it is finished with so it can be closed when everything is finished with it. We would need to keep the normal close method so that a program can force close the world.

Everything up to now has been fine because only one element should be running at once but we should plan in case we want to allow multiple level tabs to run at once (eg allowing dragging a level tab out of the standard view)

Additional context

We should discuss the solution to this problem because I am sure my solution will have problems I have not thought of and this may have been solved before.

Implementation

The code that "owns" the level should close it when it is finished with it. The __del__ method should also call the close method to make sure it actually gets closed.

[gentlegiantJGC]

If the data on disk has changed since the instance was created the data inside may no longer be correct.

[Podshot]

This is a quick and rough implementation but I believe it covers most of what is described here. It minimally changes the existing code and has reference tracking so after all references of a single world have been closed (from the perspective of Amulet Editor or whatever is asking to load said world) it will actually close the world (from the perspective of the Amulet API). I haven't added in atomic locking for when multiple threads are using the cache but that should be fairly simple to implement if this approach is taken. However, this doesn't handle the situation where data on the disk has changed (from either a second instance of Amulet API or from another program) but this could be solved by using the session.lock file locking at this cache level instead of at the AnvilFormat level (and whatever the equivalent locking logic is for Bedrock)

from __future__ import annotations
from typing import Dict, NamedTuple
from amulet import load_level
from amulet.api.level.base_level.base_level import BaseLevel

_cache: Dict[str, WorldCacheEntry] = {}

class WorldCacheEntry(NamedTuple):
    world: BaseLevel
    references: int

def wrap_close_method(path, world, old_close):
    global _cache

    def func():
        if path not in _cache: # This is needed since the BaseLevel.__del__ will be called when the actual BaseLevel object is garbage collected, which then calls this method again
            return
        _, references = _cache[path]
        _cache[path] = entry = WorldCacheEntry(world, references - 1)
        if entry.references == 0:
            del _cache[path]
            old_close()

    return func

def get(path):
    global _cache
    if path in _cache:
        world, references = _cache[path]
        _cache[path] = WorldCacheEntry(world, references + 1)
        return _cache[path].world
    world = load_level(path)
    world.close = wrap_close_method(path, world, world.close) # Decorate/wrap the close method with something that handles the cache
    _cache[path] = WorldCacheEntry(world, 1)
    return world

if __name__ == "__main__":
    world1 = get(
        "/home/podshot/Amulet-Core/tests/data/worlds_src/java/vanilla/1_18/vanilla"
    )
    world2 = get(
        "/home/podshot/Amulet-Core/tests/data/worlds_src/java/vanilla/1_18/vanilla"
    )
    print(world1)
    print(world2)
    print(world1 is world2)
    print(_cache)
    world1.close()
    print(_cache)
    world2.close()
    print(_cache)

[gentlegiantJGC]

What you have implemented could be achieved much easier with a WeakValueDictionary and just closing the level in the __del__ method. It would also close the level when the last reference is lost whereas your implementation would leave the world hanging. I started implementing this the other day but hit an issue. If we cache the format wrapper and the metadata changes on disk (edited time, level name, ...) we would return the old cached version rather than loading it again. We probably need a method to reload only the metadata to fix this.

I have been thinking while refactoring this code that it might make sense to merge the Level and FormatWrapper classes. I can't remember why I implemented it as two different classes.

[Podshot]

What you have implemented could be achieved much easier with a WeakValueDictionary and just closing the level in the del method. It would also close the level when the last reference is lost whereas your implementation would leave the world hanging.

Yeah that would be a better implementation than what I have, the only concern I have is that the logic currently in the .close() would have to move to __del__ instead of __del__ calling .close() since as it was currently implemented the world can prematurely be closed by calling .close() even though other systems may potentially have references and are expecting the world to still be open

[gentlegiantJGC]

I don't like that because if something holds a reference it will never get closed. There should be some code that is said to own the level and that is the only code that has "permission" to call the close method. When it is finished it should manually call the close method to release the level. The __del__ method should also close the level just in case it was not manually closed. The close method must not do anything if it is already closed.

It is the responsibility of the owner to make sure nothing else expects the level to be open.