Skip to content
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

PacketSwapBlock#onMessage modifies the world asychronously #143

Closed
Aaron1011 opened this issue Oct 30, 2016 · 8 comments

Comments

@Aaron1011
Copy link

Aaron1011 commented Oct 30, 2016

The packet handler PacketSwapBlock#onMessage is modifying the world directly from the handler method. Since this method is called from a Netty thread, the world is being modified asynchronously, causing problems with both Vanilla and other mods.

I recommend running this handler on the main thread, using MinecraftServer#addScheduledTask

@mbax

This comment has been minimized.

Copy link

mbax commented Oct 30, 2016

Put the entire handling section, not just the spot @Aaron1011 highlighted, into a main thread handler. You have several calls in there that could result in unpleasant or unexpected behavior.

@Lothrazar

This comment has been minimized.

Copy link
Owner

Lothrazar commented Nov 1, 2016

Good catch.

Yeah i have also had crashes with this item where it causes ConcurrentModificationException, ive been meaning to find a solution for this.

Yes i that is the exact same code used in this crash #114

@Lothrazar

This comment has been minimized.

Copy link
Owner

Lothrazar commented Nov 1, 2016

Would it help if i avoided the setBlockToAir, and just did a setBlockState to overwrite?

Ill add a scheduled task anyway

@Lothrazar

This comment has been minimized.

Copy link
Owner

Lothrazar commented Nov 1, 2016

See the referenced commit for the code changes , i think this follows your idea

` @override
public IMessage onMessage(PacketSwapBlock message, MessageContext ctx) {
if (ctx.side.isServer() && message != null && message.pos != null) {
MinecraftServer s = FMLCommonHandler.instance().getMinecraftServerInstance();//ctx.getServerHandler().getNetworkManager().get

  // MinecraftServer.getServer().//doesnt exist anymore
  if(s == null){
    handle(message,ctx);
  }
  else{
    s.addScheduledTask(() -> handle(message, ctx)); // all block changes
  }

}
return null;

}`

Ill do some testing and tidy up the code, maybe release tonight or tomorrow

@mbax

This comment has been minimized.

Copy link

mbax commented Nov 1, 2016

Other spots like dropBlockAsItem are also problematic. As are potentially calls that get blocks too, as they can potentially cause chunk loading. Also, you probably don't need that concurrent modification exception catch. :3

@Lothrazar

This comment has been minimized.

Copy link
Owner

Lothrazar commented Nov 1, 2016

yeah it didnt catch anything anyway because of async nature :P i learned that the hard way

@Lothrazar

This comment has been minimized.

Copy link
Owner

Lothrazar commented Nov 1, 2016

Thanks a lot for the code advice, i didn't know about the addScheduledTask before.

This should be fixed in build 1.7.22 https://minecraft.curseforge.com/projects/cyclic/files/2340924
Also https://github.com/PrinceOfAmber/Cyclic/releases/tag/1.7.22

I thought about doing this to every single network packet, but im not sure if that is a good idea? I would love any other advice you have to improve things here.

For getblocks, that only happens when a player uses an item, so it SHOULD be on a loaded chunk. or if a tile entity is running update() , also loaded chunk.

@Lothrazar Lothrazar closed this Nov 1, 2016
@mbax

This comment has been minimized.

Copy link

mbax commented Nov 1, 2016

If you're modifying the world in any way, you should be doing that from the main server thread. Also if iterating collections the server uses, or making any calls that might trigger loading any entities/tileentities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.