Bukkit thread safety checks and fixes

[sgdc3]

No description provided.

[ljacqu]

This would have worked out well as two pull requests—one fixing the issues you've found, and the other one with the more "experimental" feature of annotating methods / ensuring their usages.

My problem with the annotations is that it needs to be clear what they represent and when they have to be used, so that they can be maintained and kept up-to-date in the future. For example, @MightBeAsync is a little weird to me since I'm not sure what to do with that information (I'm guessing it means it's "fast" stuff that doesn't require a sync thread, as opposed to Bukkit APIs?). Might work out better to just have @Sync (or @Sync and @Async) as those really need to be respected. With @Async, I wouldn't use it too aggressively, like right now with the data source methods, none of them are called in async in the AuthMeApi and I think that's intentionally so...

[sgdc3]

Yes, but i really needed that annotations to be able to keep track of what needs to be called by the main thread and what needs to be called async.
@MightBeAsync needs to mark methods that need to be thread-safe but are not required to be called from an async context. This branch is mainly intended as a reference to properly fix these issues later.

[ljacqu]

To be honest i just really don't like it because of how spammy it is in the code. You can merge it if you want since I've anyway taken a bit more of a background role lately. I just really don't think the benefits outweigh the pain of understanding the annotations (not even going to mention the MightBeAsync name for the third time). I just think some things aren't really meant to be verified programmatically because it just doesn't... add up.

This is prone to become inconsistent, or already is. For example: a protected method annotated with @ShouldBeAsync – oh?, does everyone need to call it in an async thread or is it just because whatever public methods call it are also annotated with it? Or the data source, this will throw errors when called from the API yet we don't want to schedule those calls in there.

Edit: What does even @MightBeAsync do on private methods???

[sgdc3]

Atm we have no way to know if a method needs to be thread-safe, or if it is a blocking operation, this PR tries to add source annotation to help developers remember that.

[ljacqu]

This comment addresses none of the points I brought up.

[sgdc3]

[23:05:54 WARN]: Sync call to async method detected!
[23:05:54 WARN]: java.lang.Throwable
[23:05:54 WARN]:        at fr.xephi.authme.util.BukkitThreadSafety.shouldBeAsync(BukkitThreadSafety.java:43)
[23:05:54 WARN]:        at fr.xephi.authme.datasource.AbstractSqlDataSource.isAuthAvailable(AbstractSqlDataSource.java:34)
[23:05:54 WARN]:        at fr.xephi.authme.command.executable.authme.UnregisterAdminCommand.executeCommand(UnregisterAdminCommand.java:40)
[23:05:54 WARN]:        at fr.xephi.authme.command.CommandHandler.executeCommand(CommandHandler.java:122)
[23:05:54 WARN]:        at fr.xephi.authme.command.CommandHandler.handleCommandResult(CommandHandler.java:81)
[23:05:54 WARN]:        at fr.xephi.authme.command.CommandHandler.processCommand(CommandHandler.java:68)
[23:05:54 WARN]:        at fr.xephi.authme.AuthMe.onCommand(AuthMe.java:352)
[23:05:54 WARN]:        at org.bukkit.command.PluginCommand.execute(PluginCommand.java:46)
[23:05:54 WARN]:        at org.bukkit.command.SimpleCommandMap.dispatch(SimpleCommandMap.java:151)
[23:05:54 WARN]:        at org.bukkit.craftbukkit.v1_13_R2.CraftServer.dispatchCommand(CraftServer.java:734)
[23:05:54 WARN]:        at org.bukkit.craftbukkit.v1_13_R2.CraftServer.dispatchServerCommand(CraftServer.java:696)
[23:05:54 WARN]:        at net.minecraft.server.v1_13_R2.DedicatedServer.handleCommandQueue(DedicatedServer.java:483)
[23:05:54 WARN]:        at net.minecraft.server.v1_13_R2.DedicatedServer.b(DedicatedServer.java:440)
[23:05:54 WARN]:        at net.minecraft.server.v1_13_R2.MinecraftServer.a(MinecraftServer.java:940)
[23:05:54 WARN]:        at net.minecraft.server.v1_13_R2.MinecraftServer.run(MinecraftServer.java:837)
[23:05:54 WARN]:        at java.lang.Thread.run(Thread.java:748)

Note for myself

[sgdc3]

I would like to have an input from the other members of the team also, so we can find the better solution for this issue.

[games647]

I kind of understand both of your points. I think this idea can be useful, because it would allow us to verify the asynchronous usage. However the current state can be very misleading.

My problem with the annotations is that it needs to be clear what they represent and when they have to be used, so that they can be maintained and kept up-to-date in the future.

I agree. Therefore I think those annotations have to be documented including their motivation. Maybe it's also necessary to write a small developer introduction like in a Contributer guidelines file.

For example, @MightBeAsync is a little weird to me since I'm not sure what to do with that information (I'm guessing it means it's "fast" stuff that doesn't require a sync thread, as opposed to Bukkit APIs?). Might work out better to just have @Sync (or @Sync and @Async) as those really need to be respected.

I think Sync and Async are enough too. Having optionally async makes the situation very complex. Either it should run asynchronous, because it has to be or not.

This is prone to become inconsistent, or already is. For example: a protected method annotated with @ShouldBeAsync – oh?, does everyone need to call it in an async thread or is it just because whatever public methods call it are also annotated with it?

Good point. Maybe something like @MinecraftSync and @Blocking and annotating only public methods. Otherwise we would have to include a check into every method including private ones (like in this PR), but this could very messy. Private or protected methods then have to be encapsulated correctly that we don't change the context inside the same class.

With @Async, I wouldn't use it too aggressively, like right now with the data source methods, none of them are called in async in the AuthMeApi and I think that's intentionally so...

Our current approach is to fetch data asynchronously and then continue in this context. Taking this quoted segment further means we would to it only on heavy operations. This would allow us to reduce the number of possible thread unsafe code greatly. I'm thinking here a NodeJS similar concept:

1. Fetch user data async
2. Callback on main thread
3. Check IP async
4. Handle result on main thread

Specifically we could use something like the TaskChain project.

This would make the current code thread-safe to the Bukkit API and less complex. However requires a lot of refactoring if we consider the old code too. So I don't know how useful this can be.

[games647]

BTW: Android has a similar concept. They use annotations (@MainThread, @WorkerThread, ...) that then provide inspection errors in Android Studio (syntax only checks). This has some parallels to the proposed approach in this PR. One blog post highlights the problems they encountered: http://mcomella.xyz/blog/2016/thread-annotations.html

It shows some points that I noticed during my usage in Android development too. We should define how to handle those cases and write that down if we are going to implement it.

• Inconsistency are ignored - Forgot an annotation? - No direct usage (doesn't follow the call stack)
• How to handle wrapping methods?
• Where should we add the annotation? - Everywhere? - Only thread specific?
  • Programming overhead - New developers?
• What about API methods?