Skip to content

Fix crashes and transfer issues with item/fluid transfer (Fixes #200 & #187) #201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 6, 2025

Conversation

Rearth
Copy link
Contributor

@Rearth Rearth commented Mar 3, 2025

Refactor some things in the fluid transfer api section to prevent crashes with empty fluid variants in the fill() method. Also update the fill() and drain() methods to operate inside new threads if a transaction is already open (this happens if a mod inserts things into another inventory as part of the insert() call, such as a tesseract which does not have an internal tank. When the transfer is called from a fabric context, a new transaction is opened. This transaction context is then lost during the insert method of the neoforge mod, so FFAPI tries to create a new transaction on the same thread, which causes a crash).

This fix is inspired by the connector extras energy bridge, which also opens new threads for transactions.

This has been tested by building the mod and then doing various operations with the tesseract mod. Pipes and tanks from Mekanism, Oritech and Modern Industrialization were tested and no longer crash (while they did before in the same setup).

Also added a new check for blank item resource variants in the insertItem() calls, similar to the existing ones in extractItem(). I also got reports from this causing crashes.

@Rearth Rearth changed the title Fix crashes and transfer issues with fluid transfer (Fixes #200 & #187) Fix crashes and transfer issues with item/fluid transfer (Fixes #200 & #187) Mar 4, 2025
@Su5eD Su5eD self-assigned this Mar 4, 2025
@Su5eD Su5eD added the bug Something isn't working label Mar 4, 2025
@Su5eD
Copy link
Member

Su5eD commented Mar 4, 2025

Is this ready for review?

@Rearth
Copy link
Contributor Author

Rearth commented Mar 4, 2025

Hi, yes this is ready for review. Decided to sneak in another commit, but now its ready.

@Su5eD Su5eD merged commit 628bf5e into Sinytra:1.21.1 Mar 6, 2025
1 check passed
@Su5eD
Copy link
Member

Su5eD commented Mar 6, 2025

Thanks for fixing this btw!

@Technici4n
Copy link
Contributor

Opening transactions in a new thread is really not a good idea. This can corrupt the internal state of SnapshotParticipants because they are of course not designed for this (basically every inventory). What you would typically do instead is open a nested transaction using Transaction.getCurrentUnsafe() if there is already an open transaction.

@Rearth
Copy link
Contributor Author

Rearth commented Mar 8, 2025

Ah good to know. I did try creating a new Transaction with Transaction.getCurrentUnsafe() before going with the current implementation. However that also fails as you can't create new transactions while one is closing, and this is being called on the close event.

@Technici4n
Copy link
Contributor

Usually in that case there is nothing to be done besides not doing operations in the closing event. These events basically transition from one state to another, so allowing any operation there would complicate the implementation of the participants even more.

In this case, I would look into what is trying to perform an operation in the close event. (Well, in reality you should probably use NeoForge's native fluid API, which would avoid all of these issues and all of the ones that haven't been discovered yet. 😄 I know it sucks but that's the price of multiloader unfortunately, I did it for Powah and transfer is not fun.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants