Patches for various crashes#781
Conversation
|
Mentioned in Discord, but the Abyssalcraft tweak is not necessary, I reported it to Shinoow/AbyssalCraft#553 and it is fixed in 1.11.3/2.0.0-BETA-6. |
WaitingIdly
left a comment
WaitingIdly
left a comment
There was a problem hiding this comment.
for the documentation of the mixins, i would suggest doing it in this format:
/**
* @author RecursivePineapple
* @reason ensure index is not out of bounds
*/
@Inject(...)| @Config.RequiresMcRestart | ||
| @Config.Name("Mitigate Storage Bus Crashes") | ||
| @Config.Comment("Mitigates crashes caused by misbehaving IItemHandlers.") | ||
| public boolean uteItemHandlerCrash = true; |
There was a problem hiding this comment.
ute?
| public boolean uteItemHandlerCrash = true; | |
| public boolean utItemHandlerCrash = true; |
There was a problem hiding this comment.
typo, fixed
There was a problem hiding this comment.
this is currently always enabled - added if c.isModPresent("abyssalcraft"). if it is to be added, split it out into its own config.
There was a problem hiding this comment.
Looks like I forgot to split that off into its own mixin set, thanks for catching that
There was a problem hiding this comment.
I just removed this mixin entirely since the issue is properly fixed.
| /// at net.minecraft.util.Util.runTask(SourceFile:529) | ||
| /// ... 5 more | ||
| @Inject(method = "execute", at = @At(value = "INVOKE", target = "Lmcjty/xnet/blocks/controller/TileEntityController;createChannel(ILjava/lang/String;)V"), cancellable = true) | ||
| private void ut$fixCreateChannel(EntityPlayerMP playerMP, String command, TypedMap params, CallbackInfoReturnable<Boolean> cir, @Local(type = int.class) int index) { |
There was a problem hiding this comment.
is type = int.class actually needed here?
There was a problem hiding this comment.
I'm not sure but it's what I always do and it works fine. I don't see a reason to not add specificity.
There was a problem hiding this comment.
it doesnt do anything in this context
| /// at net.minecraft.server.MinecraftServer.run(MinecraftServer.java:526) | ||
| /// at java.lang.Thread.run(Thread.java:1474) | ||
| @Inject(method = "next()Lappeng/util/inv/ItemSlot;", at = @At(value = "NEW", target = "()Ljava/util/NoSuchElementException;"), cancellable = true) | ||
| private void ut$preventNSEE(CallbackInfoReturnable<ItemSlot> cir) { |
There was a problem hiding this comment.
i think throwing an NoSuchElementException is the correct behavior here, and the issue lies with whatever is having variable item slots between calls. to debug the cause, i would suggest doing @WrapMethod with a try { original() } catch for this in PartStorageBus#tickingRequest and logging the position it happened to.
There was a problem hiding this comment.
While I'd usually agree with you, this crash is very hard to reproduce and I'm not even sure what the actual cause is. Like I said, I can't be bothered to restart my game a few dozen times to test it. This is absolutely a hack but there's no reason that a misbehaving IItemHandler should crash the game.
There was a problem hiding this comment.
I'd like to add this: an IItemHandler reporting more slots than it has is absolutely a bug, but there's no logic error from pretending those slots exist but aren't interactable. This iterator is just used for determining what items an IItemHandler contains, so at most you're going to get false-negatives (items in storage that aren't reported to AE). Assuming the IItemHandler eventually corrects itself, this isn't even permanent because storage busses poll their attached inventories constantly.
I added some info to the AE2 mixin, but most of these bugs are trivial index errors. I don't like adding comments where they aren't necessary. I added the stacktraces to the mixins so that the mixins can be removed if these bugs are ever fixed properly. I doubt people will look at the blame to find this PR, so I didn't want to add them here. |
|
Appreciated, but I'm going to keep the AE2 tweak as opt-in for now. |
4 participants
This PR has a few patches for some frequent crashes in meatballcraft. I didn't put too much effort into root causing them, I just mitigated the crashes. I made sure my changes didn't break anything, but most of these mixins just no-op the code instead of fixing whatever's causing the issue.
Since these crashes are so sporadic, I couldn't reproduce them on-demand in my dev env. The only one I could test was the abyssalcraft crash, since that happens when you try to make an altar with the wrong blocks in the wrong dimension. I verified the others by enabling
-Dmixin.debug=trueand making sure the dumped code looked correct. Since the mixins are so simple I didn't think it was necessary to restart my game dozens of times to verify they worked properly.I just pasted the exception stack traces in the javadoc for each mixin because I wasn't sure what the best way to document them would be. The reason for the crashes are fairly self-evident when you look at the target code.
The only tricky one is UTItemHandlerIteratorMixin. As far as I can tell, it's caused by the ItemHandler's size changing while the iterator is iterating over it. I don't know what machine is causing this issue because that info isn't in my logs, so I just patched the problem by returning empty stacks and cancelled the
throw NoSuchElementException.