fast-serialization icon indicating copy to clipboard operation
fast-serialization copied to clipboard

Fixes #235 : concurrency bug in FSTStreamDecoder/FSTStreamEncoder

Open andrewl102 opened this issue 7 years ago • 6 comments

The current implementation of the cache within these classes is not thread safe, as seen by https://github.com/RuedigerMoeller/fast-serialization/issues/235 and the test case I have attached to that issue.

This might potentially have some negative performance implications but overall given the severity of the bug, I believe correctness might be more important. Proper synchronization or protection against these fields is likely possible but beyond my current understand of the codebase, as the code involved for this is quite complex.

andrewl102 avatar Jul 17 '18 14:07 andrewl102

well, this has a major performance draw back, will try using Atomic locks or so

RuedigerMoeller avatar Nov 18 '18 21:11 RuedigerMoeller

@RuedigerMoeller Any idea of when you anticipate to have time to look into this further?

perlun avatar Jan 24 '19 08:01 perlun

will have to do performance tests before merging. fst is not thread save by intent, its just too costly in high performance use cases. Even introducing some allocations on hotspot has measurable impact.

RuedigerMoeller avatar May 17 '20 15:05 RuedigerMoeller

@RuedigerMoeller is this still an issue? It looks like in the docs FSTConfiguration is meant to be thread safe. Is that not the case?

winrid avatar May 08 '22 20:05 winrid

@winrid Don't know the details about this particular bug, but see also #270 and #274 for some concurrency-related issues. We ended up moving away from FST since we couldn't get it to be stable enough for our use case. :cry:

perlun avatar May 13 '22 06:05 perlun

@perlun Thanks. Yeah it looks like a really cool project, but it's a tough problem :)

We ended up just hand rolling the serialization stuff for now because I couldn't find anything that satisfied all our requirements. Also it's a game and we are really counting bytes...

winrid avatar May 13 '22 06:05 winrid