Pithikos / python-websocket-server

A simple fully working websocket-server in Python with no external dependencies
MIT License
1.14k stars 381 forks source link

send_text() threadsafe #40

Closed playay closed 2 years ago

playay commented 6 years ago

When multiple threads call server.send_message() method at the same time, the message mixed together

Pithikos commented 6 years ago

Do you have an example that reproduces this? I tried making something up but I don't seem to get the same problem that you were experiencing..

Example:

def test_text_message_stress_bursts(session):
    """ Scenario: server sends multiple different message to the same client
    at once """
    NUM_THREADS = 20
    client, server = session

    # Threads sending different characters each of them
    from threading import Thread
    from string import ascii_letters
    threads = []
    for i in range(NUM_THREADS):
        message = ascii_letters[i]*1000
        threads.append(Thread(
            target=server.send_message_to_all,
            args=(message,)))

    # Start threads and wait for them
    for th in threads:
        th.daemon = True
        th.start()

    # Read from client
    for i in range(NUM_THREADS):
        received = client.recv()
        print(received)

    # Wait for all threads to finish
    for th in threads:
        th.join()
playay commented 6 years ago

server.py

from websocket_server import WebsocketServer
from threading import Thread
import json
import time

def keep_send_json(client, server, message):
    while True:
        time.sleep(0.01)
        server.send_message(client, message)

def new_client(client, server):
    message = json.dumps({'message': 'xx' * 1024 * 100})
    Thread(target=keep_send_json, args=(client, server, message)).start()

def message_received(client, server, message):
    server.send_message(client, json.dumps({'ss': 'ss' * 2014 * 512}))

PORT=9001
server = WebsocketServer(PORT)
server.set_fn_new_client(new_client)
server.set_fn_message_received(message_received)
server.run_forever()

client.html

<script>
ws = new WebSocket("ws://localhost:9001/");
setInterval("ws.send('xxx')", 100);
</script>

on chrome, i get this:

on safari, i get this:

vmandke commented 5 years ago

This can happen theoretically right? Even I am working on a scenario in which this might happen. As writing to socket is not thread safe... Any reason why this change wasn't merged ?

@playay @Pithikos can we use lock instead of bool as there might be race conditions while reading bool values..

playay commented 5 years ago

Actually, I don't know the principle. But it does works for me.

Pithikos commented 3 years ago

I feel the right approach to solving this would be to use a lock as @vmandke mentioned. I think that would simplify the fix, since we won't need the variables in the current fix (which adds more logic to deal with).

Pithikos commented 2 years ago

This has been fixed in v0.6.1