conqp / rcon

Python RCON client library
GNU General Public License v3.0
81 stars 15 forks source link

Multi threaded use of RCON #13

Open gilesknap opened 2 years ago

gilesknap commented 2 years ago

Should RCON be thread safe if you open a new client per thread?

I have been successfully using this but have started to see a few instances of where the response that should go back to thread A is concatenated to the response for thread B, Thread A gets a null response when this happens.

A minimal demo of the issue is here:

import threading
from time import sleep

from mcipc.rcon.je import Client

from mciwb.nbt import parse_nbt

def get_player_pos(name: str, wait: float):
    client = Client("nuc2", port=30555, passwd="spider")
    client.connect(True)
    for i in range(100):
        x = client.data.get(entity="@p", path="Pos")
        print(name, parse_nbt(x))
        sleep(wait)

def go():
    t1 = threading.Thread(target=get_player_pos, args=("t1", 0.2))
    t2 = threading.Thread(target=get_player_pos, args=("t2", 0.3))
    t1.start()
    t2.start()

The results of this test show a clash on the first call to both threads. If I adjust the wait to be the same I see more clashes.

In [1]: go()

Error parsing NBT text: TransformerScorn has the following entity data: [430.74282982933767d, 178.11815687065814d, -1507.2116496515248d]TransformerScorn has the following entity data: [430.74282982933767d, 178.11815687065814d, -1507.2116496515248d] 

 ERROR: Extra data: line 1 column 62 (char 61)
t2 None
t1 [430.74282982933767, 178.11815687065814, -1507.2116496515248]
t1 [430.74282982933767, 178.11815687065814, -1507.2116496515248]
t2 [430.74282982933767, 178.11815687065814, -1507.2116496515248]
t1 [430.74282982933767, 178.11815687065814, -1507.2116496515248]
t1 [430.74282982933767, 178.11815687065814, -1507.2116496515248]
Error parsing NBT text: TransformerScorn has the following entity data: [430.74282982933767d, 178.11815687065814d, -1507.2116496515248d]TransformerScorn has the following entity data: [430.74282982933767d, 178.11815687065814d, -1507.2116496515248d] 

 ERROR: Extra data: line 1 column 62 (char 61)
t2 None
t1 [430.74282982933767, 178.11815687065814, -1507.2116496515248]
t2 [430.74282982933767, 178.11815687065814, -1507.2116496515248]
t1 [430.74282982933767, 178.11815687065814, -1507.2116496515248]
t2 [430.74282982933767, 178.11815687065814, -1507.2116496515248]
gilesknap commented 2 years ago

14 has fixed this issue. The library can now do crazy stuff like this, tracking every currently airborne snowball in a separate thread :-)

https://github.com/gilesknap/mciwb/blob/bfbb339ad44666b841478b517d61c1159db325fd/src/demo/snowball.py#L1

gilesknap commented 2 years ago

@conqp do you get why the code above has an issue? I've looked in Client and it creates its own socket. Thus if I have two Client objects they are using separate sockets and there should be no crossover.

Unless Minecraft's RCON server itself is at fault?

gilesknap commented 2 years ago

I can confirm that adding global lock to mcipc Client fixes the above. BUT if I run two separate processes together then I get the same issue back again.

I think this implies the issue does lie at the server?!!??

BreathXV commented 3 months ago

Was just about to inquire about this as my application uses multi threading to handle each client.

gilesknap commented 3 months ago

@BreathXV are you seeing any issues with multi threading? Did you need to do anything to mitigate the problem I'm seeing?

conqp commented 3 months ago

If each thread has its own client this should™ not be an issue. In any case, I'm currently on vacation for the next two weeks, so I cannot work on this before 2024-06-03.

BreathXV commented 3 months ago

@gilesknap No, I haven't fully tested my application yet but I've been running each instance of the client as a thread, then using it to execute commands, then I start a new thread for another client.

I'll do some testing at some point today and let you know.

gilesknap commented 3 months ago

I'll do some testing at some point today and let you know.

If you do see problems then I would expect the pr I referenced above would be a workaround (but implemented at the wrong level, really)

However my testing showed the same issue even with that fix if I ran two clients on separate workstations. This implies the rcon server itself has an issue but that is rather surprising.