I had a look over your source and I’m concerned by the absolute lack of thread management which is going on. You are accessing resources from multiple threads which may work under some circumstances, but is almost certain to explode once the number of requests/server load exceeds even the most basic thresholds.
No marshalling of consumed chat messages. Chat messages are written to a non-thread-safe collection (specifically
ArrayList) which is returned “bare” to your servlet on another thread. If a chat message arrives whilst your servlet is crawling the list then it’s going to explode with a CME.
This is exacerbated by the fact that you are doing no tombstoning/size limiting of the
chatMessages list, so it will grow forever until your server runs out of heap space (unlikely but still a massive waste of memory) plus the fact that if your servlet tries to return 1GB of chat logs in a single response then (A) something will explode and (B) the longer the servlet takes to consume the chat log, the more likely the above CME becomes.
No marshalling of dispatched commands, you dispatch commands in the jetty thread which has nondeterministic behaviour. Commands should never be dispatched anywhere but the main thread.
Marshal chat messages via a thread-safe bounded queue, or at least lock the original list and return a copy of the contents when requested. Limit the length of the chat message list by cleaning entries by age or limiting queue size. You can use
ConcurrentLinkedQueue for the former (intrinsic thread-safe) or use
LinkedList (which implements both
Deque (aka double-ended queue, pronounced “deck”)) and use a monitor lock to control access to it (explicit thread-safe).
Marshal commands onto the main thread via the scheduler or pump across threads using your own
My guess is that you’re new to writing multi-threaded code so this is intended as constructive feedback. It is most likely that none of the issues I’ve mentioned have bitten you on the arse … yet … and I would guess that you’re testing in a limited environment where you’ve either been lucky enough to avoid a CME by low volume of calls, and/or that one or two have occurred but it doesn’t look repeatable enough to worry about or - worse - is just swallowed somewhere by the runtime.
The general approach when writing multi-threaded code is to treat all interactions between threads with respect. This means that you either do what’s known as affectionately as “pumping” messages from one thread to the other via a thread-safe intermediary - eg. a thread-safe queue - or use monitor locks to let both threads access the same data but only one at a time. Ignoring the problem and hoping it will go away is not a viable approach unfortunately, oh that wishes could come true of course.
A good attitube to have when writing threaded code is to do the following:
- First, try to scare yourself absolutely shitless about the world exploding if your threads screw each other over
- Once you’ve recovered from the shock, have a cup of tea or something to help calm you down
- Panic because you realise that you haven’t actually fixed the problem yet, you’ve been too busy drinking tea
- Begin writing code, safe in the knowledge that the paranioa thus acquired will help you write thread safe code
- Ship your application
- Cry in the corner as CME-related bugs trickle in anyway
- Die miserable and alone, realising that nobody ever truly understood your threads and lamenting the fact that you didn’t either.
Some key observations being:
- If you aren’t scared and paranoid when writing threaded code, you’re doing it wrong
- See point 1
As I see it there are two possibilities:
- You didn’t actually realise you were writing threaded code, and therefore didn’t account for it. This is understandable because if you’ve been used to writing plugins in the past where everything is intrinsiscally thread-safe, you may have assumed that jetty would be more of the same, with everything originating from the same thread and therfore you can act with impugnity. This is a perfectly understandable assumption but unfortunately not true.
- You realised that the code was threaded, but hadn’t appreciated that the issues I’m describing could occur, either believing that the sponge would take care of the problems for you internally, or that the chances of a problem occurring were very small and could therefore be ignored, this is also not true.
The practical upshot is this:
- Your plugin is sinking events on the “main” thread, this is fine. You are pushing chat messages into a local memory store which is fine but needs some size limiting, this is a simple issue to fix.
- You are spinning up a jetty server which of course operates on its own thread and so requests will be handled on one of jetty’s threads.
- Problems arise because a collection which is being written by one thread (the main thread) is then being accessed in a non-threadsafe way from arbitrary (potentially multiple simultaneous) jetty threads. If one of those threads is iterating over the list when a new chat message arrives, then the jetty thread is going to throw a CME.
- You are also dispatching commands from arbitrary jetty threads. The problem here is that sending commands is not thread safe either, the
SendCommandEvent thus raised is going to be raised on your arbitrary jetty thread and thus any plugin sinking that event could cause corruption if it accesses things which are gamestate, since gamestate must only ever be modified on the main thread.
I would recommed the following:
- Learn about thread-safe queues in Java, of particular interest should be
ConcurrentLinkedQueue as it’s a handy two-lock queue implementation which is good for marshalling
- Learn about monitor locks and the
synchronized keyword in Java
If you require any further advice on how to address your problems, feel free to ask in