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

fixed mongo write race condition #363

Closed
wants to merge 3 commits into from

Conversation

OmegaWulf
Copy link

Still some exceptions related to serialization but sync + migration at least works now.

@WillFP WillFP changed the base branch from master to develop July 14, 2024 23:37
Copy link
Member

@WillFP WillFP left a comment

Choose a reason for hiding this comment

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

Thanks a lot! The data handlers could do with a bit of a rewrite but this is super helpful and I'll merge this if it's all good.

Quickly though, have you tested this? I'm not 100% that line 79 will set things correctly. Worth mentioning that eco stores profile data in-memory, so you have to restart the server to check if changes have been written to the db.

Also just a minor codestyle thing; instead of doing this.plugin = plugin as you would do in java, just change the constructor to this:

class MongoDataHandler(
    private val plugin: EcoSpigotPlugin,
    private val handler: ProfileHandler
) : DataHandler(HandlerType.MONGO)

@OmegaWulf
Copy link
Author

Ah thanks for the constructor tip. I don't have experience with Kotlin.

So the original mongo data handler doesn't work at all for me. It creates an empty "data" field but never populates it. And migrations just timeout the server until it crashes. With the changes I made, it does properly migrate/save and I think it matches the intended format.

But it creates a new issue where the blockBreak listener causes exceptions (probably every listener related to gaining xp). Something wrong with serializing ObjectID into String.

I see the way Kotlin handles serialization is very different from the java mongo serialization so I'm still messing with it to see if I can figure it out.

@OmegaWulf
Copy link
Author

@WillFP okay so I just rewrote the whole data handler because I couldn't figure out the serialization with Kotlin. So all data now reads/writes to mongodb in the same format as before.

There are 2 current problems:

  1. Migration: only skill_xp is migrating for some reason. If player has farming lvl 10 with 3175 xp, then "farming_xp" will migrate but the "farming" key won't be saved to db at all. So when player joins, eco will set the players level to the minimum skill level that satisfies this xp threshold. In this example, level 7 farming is possible with 3175/5000 xp so that's what the level is saved as. Seems to be an issue beyond the mongo handler. I logged the keys in ProfileHandler and it only migrates xp.

  2. Db reading is done on main thread and this causes a massive lag spike every time a player joins. Not usable at all in production since it kills the server for 50-100 ticks on every join. I wrote readSuspend to handle async reading but I don't know Kotlin so not sure how to update the rest of the plugin to actually read async with coroutines.

@WillFP
Copy link
Member

WillFP commented Aug 25, 2024

Hey, this has been done differently: #375

@WillFP WillFP closed this Aug 25, 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.

2 participants