RFTools

RFTools

74M Downloads

RFTools Builder block leaks chunkload tickets

DarkMorford opened this issue ยท 15 comments

commented

During an investigation into some chunkloading issues on my server, I found that RFTools had 29 tickets listed in the forcedchunks.dat file for the overworld. As we don't currently have that many quarries running - certainly not in the overworld, since we have a mining dimension - I decided a little more digging was in order.

In proxy/CommonProxy.java I see that you register a ForgeChunkManager.LoadingCallback with an empty ticketsLoaded method. However, the documentation for ForgeChunkManager states that "tickets unwanted by the mod must be disposed of manually unless the mod is an OrderedLoadingCallback instance". Since your LoadingCallback does not call ForgeChunkManager#releaseTicket, I believe that there is a resource leak. These ignored but unreleased tickets will continue to pile up until the per-mod limit in forgeChunkLoading.cfg is reached (which, by default, is 200 tickets), and then the Builder will no longer be able to request new tickets from the chunk manager.

I see two potential solutions for this issue, but perhaps you can think of another. The first is simply to iterate over the offered tickets in ticketsLoaded and call releaseTicket on each one. This would indicate to the chunk manager that RFTools does not care about the ticket and also return the ticket to the pool so that it is available for the next Builder. The other solution is to implement the OrderedLoadingCallback interface and return an empty List from its ticketsLoaded method. This tells the chunk manager that none of the offered tickets are of interest, and that none of them should be subsequently offered to the ticketsLoaded method of LoadingCallback.

commented

@McJty Where is it handled? I can patch it via Mixin.

commented

Should be in BuilderTileEntity

commented

I am seeing this exact issue on my server currently. At server startup, the following error is showing the log:

[03:52:35] [Server thread/INFO] [FML]: The mod rftools has attempted to allocate a chunkloading ticket beyond it's currently allocated maximum: 200

Builder's are unable to operate, showing this error:
image

Based on DarkMorford's description, this exactly matches the behavior he indicated.

commented

Hmm I'm a bit confused about this. When am I supposed to release this tickets? You tell me how but not when

commented

Thanks for the detailed report. I will check it out soon

commented

In an ideal case, the ticket would be released as soon as it is no longer needed (i.e., when the Quarry has finished its operation or the block is broken). This has the advantage of keeping as many tickets as possible available for future operations, but may require a fair amount of code in the Builder's TileEntity.

A simpler alternative would be to release all the tickets when they are offered to the mod at server startup. Something like:

ForgeChunkManager.setForcedChunkLoadingCallback(RFTools.instance, new ForgeChunkManager.LoadingCallback() {
    @Override
    public void ticketsLoaded(List<ForgeChunkManager.Ticket> tickets, World world) {
        for (ForgeChunkManager.Ticket ticket : tickets) {
            ForgeChunkManager.releaseTicket(ticket);
        }
    }
});

This approach requires very little in terms of code changes, but has the disadvantage of still keeping tickets allocated when they are no longer necessary. On a very busy server that is not restarted regularly, it's possible that the mod's allotment of tickets could be held up by inactive quarries, and a new operation would be unable to request one until after the server was restarted.

commented

Releasing tickets when the server starts? What's the point of that? When the server starts why would there be any tickets to release in the first place? Sorry but this totally confuses me...

commented

In the Builder I have:

@Override
public void invalidate() {
    super.invalidate();
    chunkUnload();
}

private void chunkUnload() {
    if (forcedChunk != null && ticket != null) {
        ForgeChunkManager.unforceChunk(ticket, forcedChunk);
        forcedChunk = null;
    }
}

Wouldn't it be better to do the releaseTicket() there then? I already unload the chunk at that point

commented

When a mod successfully obtains a chunkloading ticket with requestTicket or changes the ticket's NBT with Ticket#getModData, ForgeChunkManager records that allocation in the dimension's forcedchunks.dat file. This file persists ticket information even after the server is shut down. When a dimension is loaded, ForgeChunkManager reads that world's forcedchunks.dat and calls each mod's registered LoadingCallback, passing it a list of the tickets it had stored. At this point it's up to the individual mod to decide what to do with each ticket. If the ticket is ignored (i.e., no action is taken), then no chunks are loaded, but the ticket remains allocated and will be offered to the mod's LoadingCallback again the next time the world is loaded. The mod may, of course, use the ticket in a call to forceChunk (perhaps using the stored NBT data to determine which chunk(s) to load), or it could call releaseTicket, declaring that the offered ticket is no longer useful and freeing it for a future allocation. I hope this clears up the confusion for you; feel free to ask if there's something you're still not clear on.

I'm not familiar with all the different cases in which TileEntity#invalidate might be called, but it seems like a reasonable place to release the ticket. Whether at load time or when the TE is invalidated, the key point is that tickets that are no longer needed have to be explicitly released, otherwise the allocations will build up in forcedchunks.dat until the per-mod limit is reached.

commented

So you see that requested tickets are persisted even when the world unloads? Are you sure of that? I never heard of that before

commented

Quite sure. I'm actually working on my own chunkloader mod, and I've observed the behavior I've described here in my development process.

My chunkloader block requests a ticket from ForgeChunkManager when it's placed in the world. If I then stop the server, I can look in the forcedchunks.dat file with an NBT editor and see the ticket there, along with the metadata I added recording the block's position. When the server starts up again, that ticket gets passed to my playerTicketsLoaded method and then to ticketsLoaded, where I use it to force-load the chunks surrounding the saved block. If I break the block (which unforces the chunks and releases the ticket), it no longer shows up in forcedchunks.dat.

commented

Over a year later this issue still exist, exactly how it's described above.

commented

2020 and builder still does not clean up after it's self. I am playing the MC.eternal using rftools-1.12-7.73.jar and with one builder(quarry clear card) the forcedchunks.dat listed over 200, causing the builder to stop working (chunk not available.). Deleting the forcedchunks.dat seems to have fixed it for now.

http://prntscr.com/rt7c1y

commented

Is there any update on this issue? We have the same problem on our Network. RFTools is just not deleting these tickets. Our server only had around 6TPS with these tickets. Deleting the forcedchunks.dat resulted that in a 10TPS increase, with over 17 players.

commented

I no longer develop on 1.12 so this is not going to get fixed. However there is a workaround. In rftools.cfg simply set quarryChunkloads to false. That will make it so the builder no longer tries to chunkload. You can then use other tools/whatever to chunkload