CraftBukkit executes commands on the server thread, unlike Vanilla, so that example is wrong.

Additionally the semantics of how this data structure is used in Vanilla/CraftBukkit prevents it from specifically being an issue as removal is only done by a single thread, which combined with the isEmpty check means that behaviour is safe within the general contract of CHM.

You are welcome to provide a counter example actually demonstrating the issue occuring and posing an issue on CraftBukkit if you wish.

(an please at least read the report)

Quote:
Conversely, executing commands via Rcon that iterate over internal data structures (such as /save-all) may cause a java.util.ConcurrentModificationException in the Rcon thread if the main thread modifies those structures (see MC-68374).

See https://hub.spigotmc.org/jira/browse/SPIGOT-1097?focusedCommentId=14366&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14366

I don't think just because a command was/(is no longer afaik) broken qualifies to disregard this.

That stack trace is completely different. Not even in CHM.

Just found the code is actually called and results in exactly what was described here. See https://bugs.mojang.com/browse/MC-72390
Using vanilla save-all command will cause this problem.

Impossible to cause in stock server.

That Crash is unrelated to this ticked: Its in another place and uses a different technique. Looks like something on your server is asyncronously manipulating the TickList (Redstone, TNT etc) resulting in that Exception. But the problem is in a theoretical view very similar.

I've experienced a crash related to NoSuchElementException.

Crash report: http://pastebin.com/c4gPnLVy
Log: http://pastebin.com/aNtEVXrL

If it helps, my server does /save-all every hour.

Right. The reason I mentioned the old code is because simply reverting the changes to the previous version works without these problems. That would be the fastest "fix". The better one would of course be to analyze the problem in depth (as we kinda did now) and fix the actual problem in the current code.

Regarding the ConcurrentMap Iterator and thread-safety: I think were hairsplitting on words now but the difference we are talking about is:
Multiple Iterators over the same Map but still only being used by one Thread each (aka single-threaded usage as you stated above) is allowed.
"Iterators are not Thread-Safe even if using a ConcurrentHashMap" would still be valid as the Iterator in it self is not thread-safe. That was my intention to say (but of course in this specific case the iterator is single-thread only).

Ok I can see the potential for NoSuchElementException now, but only in cases of custom code or the broken and not currently usable /save-all flush.

So, I doubt anyone is going to care to fix it / add diff for something that can't happen under current situations.

If you want to PR, it simply should get an instance of the iterator then check hasNext() before calling next, instead of your proposal that the entire design of the code needs to be changed.

Your ticket accused ConcurrentMap Iterators of not being thread safe, which is untrue.

The only problem here is Mojangs use of iterator().next() w/o calling hasNext(), which just an overall no no for Iterator usage. The Thread Safety elements of ConcurrentHashMap are perfectly fine here.

1.) Yes i already stated its a design flaw in the initial post. As this jira only allows 'Bug' or 'New Feature' i chose Bug and stated additional Information in the Topic. I dont see why report on a bad code-design that would break as soon as you use it as intended would be a bad thing?

2.) If the Code was in fact written properly your argument would be correct but there is no .hasNext() call - thats why i called out the design-flaw and this topic exists? The code directly calls .next() on the Iterator.

1) So you've confirmed this is not a bug in Spigot/Minecraft, as it is not possible to replicate this under supported circumstances (see #2 to confirm there still is no problem in this code). That means there is no reason for Spigot to add patches to a non broken system to support unsupported operations that are only occurring on a single server due to their modifications.

If you are witnessing any form of problem with this code due to your own changes, you're going to have to privately fix it in your own fork.

2) The code and docs disagree with you. The access to the collection is thread safe. The Iterator object itself is not thread safe. The iterator object is not passed around. I just personally inspected the JVM's code for ConcurrentHashMap and confirmed .next() CAN NOT NPE if hasNext() has returned true.

The iterator has its own LOCAL reference to the next node. There is no possibility that this 'next' reference can change when a single thread is the only thing operating on that instance of the Iterator, which is covered in the documentation.

The only questionable behavior that can occur due to this is the 2nd call to c() could potentially still return a ChunkCoordIntPair that was removed by the first one, however even this is still safe, as the remove call is written as:

NBTTagCompound nbttagcompound = (NBTTagCompound) this.b.remove(chunkcoordintpair);

remove is thread safe, and the 2nd call is guaranteed to return null, meaning the chunk will not double save.

if you're encountering thread safety issues, it is due to your modifications.

You are correct that currently c() is only called in a way that cannot produce the problem. And the problematic method is .c() and not .saveChunk() as stated in the report - i will correct that. Edit: Or not as im not allowed to edit the report

However the whole point of this report is that it will break as soon as c() is called from anywhere in the code. As you said yourself flushSave() already provides that possibility. The intention to use it is already in the code.

What are you going to get an npe on? The map will always be set. It doesn't matter if you call the code from two different threads at same time, they each will get a unique iterator that is safe to use on the thread it was created on.

A nullpointer can occure because .next() results in a null-value as the Map has already been emptied by another worker. There is no null-check in c().

Studying this more now I'm home I can see the code called in exactly 2 places:
1 - The Async File Save thread
2 - /save-all flush

.... which #2 is a broken command that will crash your server with an infinite loop.

So no there's no issue at all here. c() is only ever called from the FileIOThread.

What are you going to get an npe on? The map will always be set. It doesn't matter if you call the code from two different threads at same time, they each will get a unique iterator that is safe to use on the thread it was created on.

This is what the benefit of a concurrent hash map is, to handle all sync for you and not require you to do it yourself

Actually you're wrong. Iterators ARE thread safe AS LONG AS they are only used by a single thread and not SHARED.

Your special case contradicts the very definition of thread-safety. See https://en.wikipedia.org/wiki/Thread_safety
The cited source only refers to CME. You can still get NPE. Please review the report. I specifically state the code gets called by another thread.

(OT ChunkSaving: Thats not what im referring to. Spreading out processChunkGC() and forcing actual File-Updates is closer to what im talking about.)

Actually you're wrong. Iterators ARE thread safe AS LONG AS they are only used by a single thread and not SHARED.

Similarly, Iterators and Enumerations return elements reflecting the state of the hash table at some point at or since the creation of the iterator/enumeration. 
They do not throw ConcurrentModificationException. However, iterators are designed to be used by only one thread at a time. 

So it is not an issue. Something else must be triggering your issue.

Also

 Specific to this problem i am using a chunkSavingQueue that distributes the regular worldsaving over a longer period of time to prevent lags and save all worldchunks more frequently. 

I've already got a change into Spigot that does this sort of.

https://hub.spigotmc.org/stash/projects/SPIGOT/repos/spigot/browse/CraftBukkit-Patches/0070-Improve-AutoSave-Mechanism.patch#22

Though limiting # of chunks saved per auto save isn't a bad idea. can just add a && savedThisTick < maxPerTick check

Your build is not the latest and therefore may be the reason you are having this issue. Spigot is 2 version(s) behind. CraftBukkit is 2 version(s) behind. This message was automatically generated and is not guaranteed to be a solution to your issue.