ThreadSafe type’s value property?

[mattmassicotte]

Hello! Was just glancing though the sources, and I took note of the ThreadSafe type. It looks to me like the value property allows you to escape the queue. Am I understanding correctly?

[pepicrft]

Hi @mattmassicotte 👋🏼
Are you referring to this property? withValue should be the one protecting against a data race, but I might be missing something. ( cc @waltflanagan )

[mattmassicotte]

It does provide a consistent read, but it returns the value. From that point on, it's no longer being protected by the queue. This could be safe, depending on/why it is being protected. But, it is not safe in the general case.

To provide full safety, you would have to always access the value externally using withValue.

[waltflanagan]

Correct, if you're working with mutable reference types there is a window for a race conditions there unless they also enforce internal thread safety. I forget if there was a specific need for this in Command or if this was a result of copy/pasting from Tuist or Xcodeproj. there are many cases where we only need to read and work with a value and not mutate it and it's easier and more readable to use let foo = bar.value vs let foo = bar.withValue { $0 }.

For mutating structs though, one would only be able to set the result in a later call to bar.withValue { $0 = mutatedValue }

As is it's intended to be easy read and after that the value (should be) disconnected from the type using ThreadSafe.

100% open to questions or feedback on this approach as is, naming and threading are hard 😆

[waltflanagan]

That said, it's probably a good idea to keep this type internal since it's not intended to provide threading guarantees to consumers of Command and it's only meant to be within Command to help with safe output collection.

[mattmassicotte]

there are many cases where we only need to read and work with a value and not mutate it and it's easier and more readable to use let foo = bar.value vs let foo = bar.withValue { $0 }.

Unfortunately, neither of these constructs are safe in the general case. Once foo has escaped the protection of the queue, its internal structures are now subject to data races.

Now, it could be that this turns out to be fine! The compiler cannot reason about all possible uses. But, as a rule, a type like this that permits a protected value to escape the protection is usually a problem.

[pepicrft]

I completely missed the fact that the returned type needs to be protected. In the context of how the utility is used by Command, since we mutate it within the block to concatenate the output, it should be safe, no?

I'll go ahead with @waltflanagan's suggestion to change the visibility of this utility.

But, as a rule, a type like this that permits a protected value to escape the protection is usually a problem.

Out of curiosity, how would you recommend tackling scenarios like this? I'm still early in my understanding of strict concurrency, so I'm not entirely sure what the best practices is here:

• Should we add some annotations to indicate that the returned object is not data-race-free?
• Should we iterate in the pattern to get closer or achieve data-race safety?
• Can we solve the need that Command has without resorting to such utility? (i.e. concatenating strings coming from various streams of events).

Thanks a lot folks ❤️