StatsN
StatsN copied to clipboard
TryStatsN
The library is not thread safe when using Tcp provider
In the Tcp provider class, in method SendAsync you use Stream.WriteAsync of NetworkStream without any synchronisation. Only if you use the buffer option it is thread-safe
according to the NetworkStream doc
Read and write operations can be performed simultaneously on an instance of the NetworkStream class without the need for synchronization. As long as there is one unique thread for the write operations and one unique thread for the read operations, there will be no cross-interference between read and write threads and no synchronization is required.
This part makes me believe its ok:
As long as there is one unique thread for the write operations and one unique thread for the read operations
because we never read from the pipe at all, we only send
^ @barakakha I left you a reply but I didn't hear back. Closing due to inactivity.
Sorry for the delay.. I tested it in scale (1000 metric/sec) and it’s not thread safe. We got a lot of scrambled packets in the receiving server. This is because you have multiple writers.
Even the implementation with the queue has some issues that the background worker just stop working and not running again which make the queue overflow and consume all the memory
Do you have a repo where we can repro this? I think I get what you are saying, 2 threads are trying to concurrently write to the same stream which is causing the bits to get scrambled.
I never used the TCP implementation, we ran UDP, and eventually our own transport, its likely to be the more untested at scale client.
would you mind if i will send Pull Request to fix it using Blocking Collection?
@TerribleDev can i send a pull request? or maybe just send you the changes (only in the Tcp class)?
@barakakha definitely send over the TCP class stuff. I'd be worried about using a blocking collection as the threads will essentially block each other. The buffer stuff was never really well thought out, it certainly could use a re-do.