Skip to content

Instantly share code, notes, and snippets.

@drewmccormack
Created February 2, 2019 10:25
Show Gist options
  • Star 7 You must be signed in to star a gist
  • Fork 1 You must be signed in to fork a gist
  • Save drewmccormack/b1c4487935cf3c3e0a5feaf488a95ebd to your computer and use it in GitHub Desktop.
Save drewmccormack/b1c4487935cf3c3e0a5feaf488a95ebd to your computer and use it in GitHub Desktop.
Demonstrates how a Swift value constant can mutate when using Copy-on-Write (CoW) and multi-threading.
import Foundation
struct NuclearPowerStationOperator {
class Storage {
var turnOffCores: Bool = false
func copy() -> Storage {
let new = Storage()
new.turnOffCores = turnOffCores
return new
}
}
private var storage: Storage = Storage()
var turnOffCores: Bool {
get {
return storage.turnOffCores
}
set {
if isKnownUniquelyReferenced(&storage) {
Thread.sleep(forTimeInterval: 1.0) // Sleep to simulate race condition
storage.turnOffCores = newValue
} else {
storage = storage.copy()
storage.turnOffCores = newValue
}
}
}
var description: String {
return "\(turnOffCores ? "We are in danger" : "We are safe")"
}
}
// Create a mutable value
var crazyOperator = NuclearPowerStationOperator()
DispatchQueue.global(qos: .background).async {
Thread.sleep(forTimeInterval: 0.5) // Sleep a little to give main thread time to start setting property
let saneOperator = crazyOperator // Create a constant copy of the operator from main thread
print(saneOperator.description) // Print our intial property value
Thread.sleep(forTimeInterval: 2.0) // Simulate race by waiting for setter on main thread to finish
print(saneOperator.description) // Test property (it will be different)
}
// Update the value. Note that the setter simulates a race condition by being very slow
crazyOperator.turnOffCores = true
@helje5
Copy link

helje5 commented Feb 2, 2019

Again to quote @jckarter: https://twitter.com/jckarter/status/975729078354395137

It’s thread safe to read and copy but not write (modulo bugs). It should be as thread safe as an int variable

What you are demonstrating here is this, just in complicated:

int i = 0;
otherThread {
  printf("%i\n", i);
}
i += 1

While that works most of the time, it is incorrect. You either need to do:

NSLock *mutex = [[NSLock alloc] init];
int i = 0;
otherThread {
  int myI;
  [mutex lock];
  myI = i;
  [mutex unlock];
  printf("%I\n", myI);
}
[mutex lock];
i += 1
[mutex unlock];

or use another synchronisation mechanism, like AtomicIncrement. All value types work just like Int here, regardless whether they use CoW or not.

@helje5
Copy link

helje5 commented Feb 2, 2019

To adjust your example, this is one way to protect the concurrent write to shared state:

let lock = NSLock()
var crazyOperator = NuclearPowerStationOperator()

DispatchQueue.global(qos: .background).async {
    Thread.sleep(forTimeInterval: 0.5) // Sleep a little to give main thread time to start setting property
    lock.lock()
    let saneOperator = crazyOperator // Create a constant copy of the operator from main thread
    lock.unlock()
    // Note that the saneOperator is now thread-safe!
    print(saneOperator.description) // Print our intial property value
    Thread.sleep(forTimeInterval: 2.0) // Simulate race by waiting for setter on main thread to finish
    print(saneOperator.description) // Test property (it will be different)
}

// Update the value. Note that the setter simulates a race condition by being very slow
lock.lock()
crazyOperator.turnOffCores = true
lock.unlock()

@drewmccormack
Copy link
Author

I'm afraid I disagree. The CoW example is not equivalent to a true value type, like an Int, in this case. Here is your simplified version rewritten to be analogous to my CoW example.

var i: Int = 0
DispatchQueue.global(qos: .background).async {
    let o = i
    print("\(o) original value copy", o)
    usleep(10)
    print("\(o) final value of copy", o)
}
i += 1

The two print statements will always print the same value here. Whether that is the value you want is a different question, but once o has been assigned, in the let, the memory from i will have been copied into private memory belonging to o, and will not change thereafter. let means let here.

In my CoW example, let does not mean let. There is a race condition in CoW types, which is not possible with true value types, where no state is shared. The fact that the CoW value can actually change, even though we have used let, is precisely because it is sharing state with other copies.

@helje5
Copy link

helje5 commented Feb 2, 2019

Yes, how they fail is different because the implementation is different. Yet both versions are incorrect and lead to junk. If your point was to demonstrate that CoW structures being used incorrectly lead to different junk than Int when being used incorrectly, that is fair! But I'm not sure how helpful that is :-)

The value-type in Swift promise for both Int or a CoW based value type is:

It’s thread safe to read and copy but not write (modulo bugs). It should be as thread safe as an int variable

Unlocked writing to shared state may yield to different programs failures, but both are equally buggy and undefined. And in fact the same is true for many more variants of value types, not just CoW types (like a struct { char a[10]; }, or Int's actually being pointers will react differently to garbage, etc).

@jessearmand
Copy link

jessearmand commented Feb 2, 2019

A similar implementation in Rust would be the following:

use std::thread;
use std::time::Duration;

struct NuclearPowerStationOperator {
    turn_off_core: bool,
}

fn main() {
    let mut crazy_operator = NuclearPowerStationOperator {
        turn_off_core: false,
    };

    let handle = thread::spawn(move || {
        thread::sleep(Duration::from_millis(500));

        let sane_operator = crazy_operator;
        println!("Core is {}", sane_operator.turn_off_core);

        thread::sleep(Duration::from_millis(2000));

        println!("Core is {}", sane_operator.turn_off_core);
    });

    thread::sleep(Duration::from_millis(1000));
    crazy_operator.turn_off_core = true;

    handle.join().unwrap();
}

If I run cargo build, this is the output:

~/Development/rust/shared_state HEAD cargo build
   Compiling shared_state v0.1.0 (/Users/jessearmand/Development/rust/shared_state)
warning[E0382]: assign to part of moved value: `crazy_operator`
  --> src/main.rs:25:5
   |
13 |     let handle = thread::spawn(move || {
   |                                ------- value moved into closure here
...
16 |         let sane_operator = crazy_operator;
   |                             -------------- variable moved due to use in closure
...
25 |     crazy_operator.turn_off_core = true;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ value partially assigned here after move
   |
   = note: move occurs because `crazy_operator` has type `NuclearPowerStationOperator`, which does not implement the `Copy` trait
   = warning: This error has been downgraded to a warning for backwards compatibility with previous releases.
           It represents potential unsoundness in your code.
           This warning will become a hard error in the future.

    Finished dev [unoptimized + debuginfo] target(s) in 2.33s

Then if I run the executable with cargo run

~/Development/rust/shared_state HEAD cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.54s
     Running `target/debug/shared_state`
Core is false
Core is false

The difference is with Rust, a data type needs to implement Copy trait explicitly to have copy-on-write. By default the crazy_operator is moved to the thread closure. I haven't tried to implement Copy trait for this. Also there's a similar concept: Clone trait which is done on the heap allocated data, while Copy is done on a stack allocated data.

Regardless, the build warning shows you that the assignment there should not happen.

@ravikandhadai
Copy link

ravikandhadai commented Feb 4, 2019

Nice illustration of an anti-pattern for implementing COW types. At a high level, it seems the best fix would be to ensure that the check isUniquelyReferenced(&storage) and the updation: storage.turnOffCores = newValue happens atomically. That is, no context switch should be possible between the two.

@Lukasa
Copy link

Lukasa commented Feb 4, 2019

@ravikandhadai A context switch is not relevant. The relevant part is that if you share a reference to a value type, and concurrently read/write to that reference from multiple threads, your program is buggy. That's the end of the discussion.

Specifically, the behaviour here has racing reads and writes from multiple threads without synchronisation. As @helje5 points out, that's not safe to do with any value, ever.

You can see this clearly by translating @drewmccormack's simplified example on Int:

var i: Int = 0
DispatchQueue.global(qos: .background).async {
    let o = i
    print("\(o) original value copy", o)
    usleep(10)
    print("\(o) final value of copy", o)
}
i += 1

into C:

int i = 0;

void test(void) {
    dispatch_async(dispatch_get_global_queue(QOS_CLASS_BACKGROUND, 0), ^{
        int o = i;
        printf("Here's the value: %d\n", o);
    });
    i = 1;
}

test();

This code is clearly not right. There is a race on the read from i and the write to i. On some architectures, this may happen to work some of the time, but it is fundamentally not thread safe. You are never allowed to race unsynchronised reads and writes from multiple threads. The fact that CoW values make this harder does not change the fact that it is simply never safe to do this.

@ogres
Copy link

ogres commented Feb 4, 2019

You have to add crazyOperator to a capture list of the closure to get the behaviour you wanted. Without it, closure will have a reference to your variable, it will not copy it on creation.

Change

DispatchQueue.global(qos: .background).async {

into

DispatchQueue.global(qos: .background).async { [crazyOperator] in

To see the difference between a closure with and without a capture list, have a look at the following code:

import Foundation

var int: Int? = 100
var array: [Int]? = [200]

DispatchQueue.global(qos: .background).async {
    let copy_int = int
    let copy_array = array
    print("Without capture list int: \(String(describing: copy_int)), array: \(String(describing: copy_array))")
}

DispatchQueue.global(qos: .background).async { [int, array] in
    let copy_int = int
    let copy_array = array
    print("With capture list int: \(String(describing: copy_int)), array: \(String(describing: copy_array))")
}

int = nil
array = nil
Thread.sleep(forTimeInterval: 1)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment