Document that fastring is not thread-safe

[idealvin]

Document that fastring is not thread-safe

[idealvin]

@snej

• Copying fastring is thread-safe like the std::string.
• copy-on-write is not used in fastring.

[snej]

Atomically updating the ref-count is not enough to make the implementation thread-safe! The copy constructor and assignment operator create multiple instances that point to the same memory, so modifying those instances on multiple threads will cause problems.

For example:

• fastring a = "......";
• fastring b = a;
• Pass b to a second thread
• On the first thread, call a.find(...) -- this will access bytes pointed to by _p->s
• On the second thread, call b.append(...) -- this may call realloc(_p->s), which will free the bytes being used by the first string

This would work fine with std::string because the copy constructor allocates a new heap block for the bytes, making the new object fully independent from the first.

It appears fastring has a clone method that creates a fully independent copy, and that could be used for thread-safety.

I don't mean to say there's anything wrong with fastring; but it has very different behavior than std::string, and using it without knowing that will cause very nasty bugs in multi-threaded code. So I'm just suggesting that you document this.

[idealvin]

Hi, Jens, thanks for your suggestion. I'll update the documents later. A little busy for these days..

[idealvin]

@snej
Hi, Jens, the document for fastring already said about it. See Special Notes in the document:

fastring s = "xx";
fastring t = s;   // s, t points to the same string
t + = 'x';        // s, t-> "xxx"
t = "888";        // s, t-> "888"

fastring r = "r";
t = r;            // t-> "r", s is still "888", only the reference count is decremented by 1

fastring x;       // empty string, no memory allocated
fastring y = x;   // x and y do not share memory since no memory is allocated
y = "yy";         // y -> "yy", x is still empty

I'm not sure if it is clear enough. You may help to improve it?

[snej]

That section helps, but it only tells half the story. If the shared string storage were thread-safe [the way std::string was long ago] there would not be concurrency issues. So the other half of the story is to point out that it's not thread-safe.

I suggest adding a section like:

Warning for multi-threaded use: Strings should never be shared between threads that might modify them, because the operations that mutate string storage are not thread-safe. To safely pass a string from one thread to another, call its clone method, which creates an entirely separate fastring with its own copy of the storage.

[idealvin]

That section helps, but it only tells half the story. If the shared string storage were thread-safe [the way std::string was long ago] there would not be concurrency issues. So the other half of the story is to point out that it's not thread-safe.

I suggest adding a section like:

Warning for multi-threaded use: Strings should never be shared between threads that might modify them, because the operations that mutate string storage are not thread-safe. To safely pass a string from one thread to another, call its clone method, which creates an entirely separate fastring with its own copy of the storage.

@snej OK. Thanks. I' ll do it later.

[idealvin]

I'll document it at the next release.

[idealvin]

@snej I have removed reference counting from fastring. Now it has a similar copying behavior to std::string.