fix: bukkit player api usage

[bridgelol]

Fixes #231

Edit by @BlitzOffline: Fixes #158

[BlitzOffline]

I do not consider this a fix as all it is doing is "hiding" the underlying issue which based on my previous research is most likely caused by Geyser and/or Floodgate. The issue might also show up in the future as there are some places where the NPE throwing method is still used.

However, if the rest of the team considers it good enough, I don't mind if it will be merged.

[bridgelol]

I do not consider this a fix as all it is doing is "hiding" the underlying issue which based on my previous research is most likely caused by Geyser and/or Floodgate. The issue might also show up in the future as there are some places where the NPE throwing method is still used.

However, if the rest of the team considers it good enough, I don't mind if it will be merged.

Highly doubt this is the issue, simply, if you look at the stacktrace and look at the root cause of this, it's concurrency.

Chat event is handled asynchronously and you're fetching a player off-main thread. This has the possibility of returning null. Fixing the ChatUser#hasPermission(String) method by returning false if the bukkit player wasn't found solves this completely.

If there are other places where the bukkit player is fetches asynchronously, it should use a similar approach of only using the bukkit player if it actually exists.

[BlitzOffline]

Highly doubt this is the issue, simply, if you look at the stacktrace and look at the root cause of this, it's concurrency.

Chat event is handled asynchronously and you're fetching a player off-main thread. This has the possibility of returning null. Fixing the ChatUser#hasPermission(String) method by returning false if the bukkit player wasn't found solves this completely.

If there are other places where the bukkit player is fetches asynchronously, it should use a similar approach of only using the bukkit player if it actually exists.

You seem to be correct. I've completely forgotten that Bukkit methods should not be considered thread-safe. That is my bad.

The code looks ok but I am wondering if the playerNotNull method should be completely removed. This way whenever someone uses the player method, they see the response is not always present and have to make a conscious decision whether to check if the Player exists.

Another note would be that the User#canSee method is sometimes called async when called from MessageProcessor#process.

[bridgelol]

Highly doubt this is the issue, simply, if you look at the stacktrace and look at the root cause of this, it's concurrency. Chat event is handled asynchronously and you're fetching a player off-main thread. This has the possibility of returning null. Fixing the ChatUser#hasPermission(String) method by returning false if the bukkit player wasn't found solves this completely. If there are other places where the bukkit player is fetches asynchronously, it should use a similar approach of only using the bukkit player if it actually exists.

You seem to be correct. I've completely forgotten that Bukkit methods should not be considered thread-safe. That is my bad.

The code looks ok but I am wondering if the playerNotNull method should be completely removed. This way whenever someone uses the player method, they see the response is not always present and have to make a conscious decision whether to check if the Player exists.

Another note would be that the User#canSee method is sometimes called async when called from MessageProcessor#process.

https://github.com/HelpChat/ChatChat/pull/232/commits/b4cc8edcab7e443dbf1c708f64459aba164b141c

[bridgelol]

good to merge then?