Skip to content

Fix NullPointerException crash in PipeFlowFluids#4656

Merged
AlexIIL merged 2 commits intoBuildCraft:8.0.xfrom
anonyco:patch-1
Oct 31, 2021
Merged

Fix NullPointerException crash in PipeFlowFluids#4656
AlexIIL merged 2 commits intoBuildCraft:8.0.xfrom
anonyco:patch-1

Conversation

@anonyco
Copy link
Copy Markdown
Contributor

@anonyco anonyco commented Oct 28, 2021

FIXES a bug where accesing lastSentDirection.nbtValue throws a NullPointerException when placing a wooden fluid pipe next to a Magneticraft Infinite Water block and then breaking wooden fluid pipe. To the best of my Java ability I couldn't figure out why :(

FIXES a bug where accesing lastSentDirection.nbtValue throws a NullPointerException when placing a wooden fluid pipe next to a Magneticraft Infinite Water block and then breaking wooden fluid pipe. To the best of my Java ability I couldn't figure out why :(
@AlexIIL
Copy link
Copy Markdown
Member

AlexIIL commented Oct 28, 2021

Thanks for the bug report - sadly my lack of javadocs didn't help much, but Section.lastSentDirection is used solely as a way to avoid sending excess packets to the client - (I.E. only send a packet to the client if anything actually changed). As such it doesn't even need to be saved!

So the real fix for this is to delete these two lines: https://github.com/BuildCraft/BuildCraft/blob/8.0.x/common/buildcraft/transport/pipe/flow/PipeFlowFluids.java#L815 and https://github.com/BuildCraft/BuildCraft/blob/8.0.x/common/buildcraft/transport/pipe/flow/PipeFlowFluids.java#L826

@anonyco
Copy link
Copy Markdown
Contributor Author

anonyco commented Oct 30, 2021

I have fixed my PR so that it now removes those two lines as you suggested.

Also, after looking at it with a fresh set of eyes, I realized the reason why it was throwing an error:

Note that I work mostly in C/C++ and dream in Lisp, so please be patient as I try to comprehend the intricacies of Java lol.

Keep up the great work 👍

@AlexIIL
Copy link
Copy Markdown
Member

AlexIIL commented Oct 31, 2021

You're nearly correct - with one exception: you can only get a NullPointerException when dereferencing a value with null - which only happens when you use . on a variable/field/method result containing null. Using the identity comparison operators (== or !=) on null is perfectly safe. As such lastSentDirection being null is actually always safe, since the only place it was dereferenced is when it was saved to disk - which needs to be removed. It's also completely harmless (and probably beneficial) to initialise it though - since Dir.NONE is what the client starts out with.

@AlexIIL
Copy link
Copy Markdown
Member

AlexIIL commented Oct 31, 2021

Basically this looks good now, thanks!

@AlexIIL AlexIIL merged commit b2d0b11 into BuildCraft:8.0.x Oct 31, 2021
@AlexIIL AlexIIL mentioned this pull request Oct 31, 2021
@anonyco
Copy link
Copy Markdown
Contributor Author

anonyco commented Nov 1, 2021

You're nearly correct - with one exception: you can only get a NullPointerException when dereferencing a value with null - which only happens when you use . on a variable/field/method result containing null. Using the identity comparison operators (== or !=) on null is perfectly safe. As such lastSentDirection being null is actually always safe, since the only place it was dereferenced is when it was saved to disk - which needs to be removed. It's also completely harmless (and probably beneficial) to initialise it though - since Dir.NONE is what the client starts out with.

Thank you for taking the time to explain this to me. I learn something new about Java every day lol.

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.

2 participants