I got one important question, is there a some indication on which method is actually ThreadSafe?
I'm not as familiar with most of the Api which is why I can't really tell what is actually ThreadSafe and what not so if there is something I should know about that I could do a lot more annotating.

Maybe someone could tell me for example which packages contain Fully Thread Safe Classes or something like that?

EDIT: Right now there is almost nothing annotated since the lack of time over the last two months

oh yeah, you're right, I made it public now

And since we didn't really start yet you can just choose a package, create a branch and start working on it
I gave you write access to the repository

Your repo is private. Also if you can write a package which I should go through, it would probably help to prevent accidentally working on the same classes.

We could also rename the second one there is no problem with that.

If you have a better name for the thread safety of mutable objects then we can use that

I'm not sure. I would've suggested something with Context but you've already used that so not sure

Any Idea on how we could name a 4th annotation and their description?
I mean if its more clear then sure it would be great

Hmmm, I guess the argument is that you will never 'own' the context of a non thread safe server object. I guess its technically true (and makes this task a lot easier) but it is less clear.

> Good question, maybe we need a 4th option

Couldn't we combine that "single thread" and "context related" things into one annotation?

> Also careful about changing formatting in your repository Laurii, that could become a headache to undo down the track.
Suggest disabling auto format in your IDE.

Yeah I know, I already created a new workspace because of our code style so that I can use a different one for spigot.
Maybe I'll also disable it, don't know yet. I'll definitely not use a cleanup or format on the whole project as that will mess things up yeah

Also careful about changing formatting in your repository Laurii, that could become a headache to undo down the track.
Suggest disabling auto format in your IDE.

> One thing however which could be hard to annotate are methods which depend on the "context" / implementation. For example RegionAccessor#spawn or RegionAccessor#setType if the RegionAccessor is implemented by the World object those methods are only save to call on the server Thread. If however the RegionAccessor is implemented by LimitedRegion it is only save to call those methods with the Thread which provided the LimitedRegion. How would you annotate such methods?

Good question, maybe we need a 4th option

@Marvin Rieple

If you want to help annotating every method with the correct annotation then you can
visit our stash repository.

Right now we only added the annotations and added them to the Annotation test but we'll soon start annotating some methods and constructors

@md_5

I think a good way of naming everything is:
NonThreadSafe
ContextThreadSafe
FullThreadSafe

I think something like that would fall in the "SingleThreadSafe" category

I like this Idea since it makes clear which method can be called on an other Thread and which not. And maybe even leads to looking at some methods and try to make them thread safe in craftbukkit.

If you link the repo / branch your working with I'm willing to add annotation to some part of the bukkit api.

One thing however which could be hard to annotate are methods which depend on the "context" / implementation. For example RegionAccessor#spawn or RegionAccessor#setType if the RegionAccessor is implemented by the World object those methods are only save to call on the server Thread. If however the RegionAccessor is implemented by LimitedRegion it is only save to call those methods with the Thread which provided the LimitedRegion. How would you annotate such methods?

That should be no problem as there is already a annotation in use in the SpigotApi called Contract which uses a String parameter

See here: https://hub.spigotmc.org/javadocs/spigot/org/bukkit/block/banner/PatternType.html#getByIdentifier(java.lang.String)

But we could also use a annotation fur each should also be no problem at all
And I think it would also be possible to unit test the type of annotation that I sent above

> Like the Thread who currently owns the object is safe to use it?

Something like that, I'm not sure of a good name.

> Or maybe for non ThreadSafe operation just no annotation at all

Better to have one on everything so we can unit test it like null/notnull.

> Also about using a annotation for this, what about this layout:

Does enum level show in Javadocs? Java convention seems to be mostly Null/NotNull instead of Nullable(true/false) but I'm not fussed if it appears cleanly in javadocs/ IDEs.

How would you define Single Thread safe?
Like the Thread who currently owns the object is safe to use it?

Also about using a annotation for this, what about this layout:

package example;
import static java.lang.annotation.ElementType.CONSTRUCTOR;
import static java.lang.annotation.ElementType.METHOD;
import static java.lang.annotation.RetentionPolicy.SOURCE;
import java.lang.annotation.Documented;
import java.lang.annotation.Retention;
import java.lang.annotation.Target;
@Documented
@Retention(SOURCE)
@Target({
    METHOD,
    CONSTRUCTOR
})
public @interface ThreadSafety {
    SafetyLevel level();
    
}

package example;
public enum SafetyLevel {
    
    NONE,
    SINGLE,
    FULL;
}

Or maybe for non ThreadSafe operation just no annotation at all

> Documenting each and every method with such a annotation will consume a lot of time

It's been done before: https://hub.spigotmc.org/stash/projects/SPIGOT/repos/bukkit/commits/3f0591ead707879a05dac22aa958af75e7e1b97e

I think there needs to be three levels of thread safety:

1. Fully thread safe (concurrent, scheduler, singletons, many constructors)
2. Single thread safe (?, mostly mutable things like ItemStack, BlockData)
3. Not thread safe (everything else)

I fully agree with your point.

Documenting each and every method with such a annotation will consume a lot of time but I think it would be good to start doing that especially since more people tend to do stuff on another Thread than the main server Thread.

I think if you agree with that opinion, we could start adding such a annotation in a PR after we signed the CLA

I think it's a whole lot more nuanced than that. Thread safe can mean a lot of things. To use the BlockData example, creating a new BlockData is thread safe in all senses. But manipulating a BlockData instance is thread safe in that it can be done off the server thread, but not thread safe in that it can be done concurrently.

I also think singling out BlockData for documentation is a bit weird. Bukkit.getServer() is thread safe too, but that's not documented. Same with many many many other methods (and a few constructors). At some point developers need to make a reasoned call and exercise thought. It's pretty clear that BlockData can be created off thread given we have an async world gen API.

If this were to be done my view is it should be done consistently by annotating every single method in the API, the way we have done with nullability. It's just otherwise not compelling to me why we would put documentation (which no one reads) in one specific method out of hundreds.

@md5 The change in the documentation would also affect some of spigots forks, that removed the Map that hold the BlockData for having 'more performance'. This in the end results in the method not beeing threadsafe. With the addition of async tasks in worldgenerators this leads to massive performance-issues as each call would need to be made sync or developers would need to store every Material theirself. So it is important, that the method is threadsafe and if spigot clearly defines the method threadsafe in their docs the forks will surely follow, as developers build for spigot and not the forks. 

> Um what do you mean marked thread safe?

With a note in the docs or maybe a annotation
Something like that

> What APIs are marked thread safe?

I don't really know if there is any really marked as Thread Safe as I didn't read the whole API but I think it would be nice if we could add such information to the docs so developers know what they can access from other threads without issues

> Especially when designing World-Generators this might give a performance boost.

Can you explain how documentation gives a performance boost?

Um what do you mean marked thread safe?
What APIs are marked thread safe?

Would definitely agree on that issue. Especially when designing World-Generators this might give a performance boost. I also see no reason why it should be not Threadsafe.