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

fix flood lights te issues #6

Closed

Conversation

Pilzinsel64
Copy link

@Pilzinsel64 Pilzinsel64 commented Dec 4, 2024

After a long time of mysterious errors we were finally able to reproduce the issues and finally fix them. Not sure if the code here is best-practice, so feel free to correct me.

From the commit message:

  • This can happen probably because the new TE will not be created fast enough after we set the block (or whatever).
  • This fix finally works on our production server.
  • It also fixes a ConcurentModificationException by using a copy of the actual sources list while it can be updated at updateAllSources

- This can happen probably because the new TE will not be created fast enough after we set the block (or whatever).
- This fix finally works on our production server.
- It also fixes a `ConcurentModificationException` by using a copy of the actual `sources` list while it can be updated at `updateAllSources`
@Pilzinsel64 Pilzinsel64 requested a review from a team December 4, 2024 21:03
@wlhlm
Copy link
Member

wlhlm commented Dec 5, 2024

Do you know what causes the CME? What are the different threads accessing sources in parallel here? ArrayList.addAll() is not thread-safe either, you can still run into CMEs during that. I feel like some synchronization is necessary if parallel access is unavoidable.

Copy link

@Cardinalstars Cardinalstars left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this going to work between server and client? As far as I understand using world.setTileEntity needs to have something special to go along with it when updating the tile entity in the description packet (because I can't remember if the descriptionPacket will create the tileEntity if it doesn't exist on the client side). I need to look into this more later when I get home. Also, I'm not sure if this code runs on server and client.

@Pilzinsel64
Copy link
Author

Pilzinsel64 commented Dec 5, 2024

Do you know what causes the CME

@wlhlm There are two possible sources. One is the due setTileEntity call I added and another one is at RemoveTileEntity.

Both are called after I clone the list. I just want to iterate a local copy of the state of before.


Is this going to work between server and client?

@Cardinalstars It works between server and client, yes. Tested this mainly on server my private prod and also on a backup server.


The main reason why I made it like it is is just that after setBlock the TileEntity isn't available yet (which leads to an "Can not cast to XXX error" at this point) and I need a quick fix for the prod server. Feel free to suggest a better way (or make a better fix, I'm just developing on MC mods / with Java casual).

@Pilzinsel64
Copy link
Author

Pilzinsel64 commented Dec 7, 2024

Closing this as the main issue seems to still be present. God I hate that ...

The main issue seems to be that Ender IO lights and Flood Lights are incompatible with each other. Means, if you place a EIO light in the near of a Flood light, you will get a server crash at some point. Both want to spread out their light TEs and both overwrite each other on loading the chunk.

Also, in combination with a light on the edge of a chunk, it crashes too.

I'm going to make a bug report as soon as I got a fully reproduceable example. Someone else need to get into this, this is absolutely out of my scope due missing time.

@Pilzinsel64 Pilzinsel64 closed this Dec 7, 2024
Pilzinsel64 added a commit to LITW-Refined/FloodLights that referenced this pull request Dec 7, 2024
@Pilzinsel64 Pilzinsel64 mentioned this pull request Dec 7, 2024
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.

3 participants