Closed Songbird0 closed 5 years ago
Kakifrucht
commented
7 years ago Funny you should mention it now, afaik @rogermb talked about this a couple of days ago on Gitter and I'm pretty sure he fixed it (Didn't commit it just yet), if I understood your problem correctly.
Songbird0
commented
7 years ago Hey you! :)
Thanks for the mention.
if I understood your problem correctly.
It's simple, supposing that 3 threads are created to listen onTextMessage() event.
Implementation:
@Override
public void onTextMessage(TextMessageEvent e){
apiInstance.whoAmI().getId(); // will get an error
}whoAmI().getId() call chain without any problem;You understood this thing, initially ? :)
rogermb
commented
7 years ago Hi again!
We actually recently had a discussion on thread safety and ordering guarantees in event listeners, as @Kakifrucht mentioned. Sorry I couldn't respond to you faster, but I wanted to take my time on this. (And I also needed sleep and had some real-life obligations 😛)
Currently, there are no thread safety or ordering guarantees in event listeners. This has been the case in this project since the very first commit.
If your event listener isn't a pure function (no side effects), then your event listener code needs to be not just sequentially, but also concurrently correct. That means that you'll need to synchronize your field accesses, make sure there are no race conditions, somehow prevent reorderings (that one's tough), etc.
This is why your code doesn't work properly. The event listener is simply running multiple times at once (for different events).
In the upcoming release of version 1.1, this behavior will change.
There will still not be "a single thread" for all event listeners, as this could negatively affect the performance too much. For example, that would make it possible for one event listener to block all other event listeners from running.
Instead, there will be a strict ordering of events per event listener. That means each event listener you register will only ever be executing one onXYZ method at a time. Furthermore, it will no longer be possible for events to get reordered.
This effectively gives you a guarantee that you can access all fields and all parts of memory that are not shared outside the event listener without requiring synchronization.
I guess this also emphasizes decoupling different event listeners and splitting them up into different classes. That way, those event listeners that don't affect each other could run at the same time, when they couldn't if they were implemented in the same class.
Apart from all that, you're also running into command timeout problems. If you're filling the command queue faster than what the socket / flood rate settings can handle, some commands will eventually time out. In the current version, all synchronous commands have a built-in "countdown timer". If it reaches 0, the command is kicked from the queue and null is returned. This, too, will change in the next release.
Kakifrucht
commented
7 years ago Thanks, I recompiled to test your changes, looks good out of the box.
Songbird0
commented
7 years ago Hi,
Sorry I couldn't respond to you faster, but I wanted to take my time on this. (And I also needed sleep and had some real-life obligations :stuck_out_tongue:)
No problem!
If your event listener isn't a pure function (no side effects), then your event listener code needs to be not just sequentially, but also concurrently correct. That means that you'll need to synchronize your field accesses, make sure there are no race conditions, somehow prevent reorderings (that one's tough), etc.
All right. I'll do my best to synchronize properly my resources. By the way, (if the logs are removed) my "solution" sounds be correct.
Thanks all!
Regards.
rogermb
commented
5 years ago Completely forgot about this issue. The threading guarantees mentioned in my last comment were added in v1.1.0, which was released almost half a year ago 😄. Closed
Hi !
TL;DR
I'm working on a somewhat stylish TeamSpeak bot and I need to listen
onTextMessageevents. For my job, no asynchronous tasks are necessary, then I useTS3Apiobject.Notice: Despite the substantial source code and logs below, please read carefully. Notice(1): Don't worry about "LE PLUS PRÉCIS" log title. It is french because my OS locales are FR.
Question: For my own,
TS3Apishould provide synchronous threads only, but it doesn't. Is it a bug or a wanted feature?I tried this code(I put this code so you can spot the problem via stacktrace):
Don't worry about french comments again, all important informations are english.
I expected to see this happen: All threads are sequentially executed.
Instead, this happened: All
onTextMessage()event threads crash because they access to null references. When these threads are executed, the channel in question is already deleted. No additionnal tests are needed.Meta
Backtrace:
First example, for the Science (!), I tried to spam my bot and look his behavior. ~12 threads were created(i.e. 1
onTextMessage()event, 1 thread/runnable created).Here, my bot is checking that I made lower than 4 errors:
Note these 2(up to 12) logs occurrences "I'LL USE BAN... KICK HAMMER! TAKE THIS!". 'Songbird' cannot be kicked several times, the server connection is throttled.
Solution
I tried to fix this problem myself by adding a
synchronizedmutex ononTextMessagemethod signature, and it works!However, like expected, my logs report a 'null' reference:
Spam:
Response:
All threads are terminated properly and there is no NPE, yay! :D