pointfreeco / swift-concurrency-extras

Useful, testable Swift concurrency.
MIT License
343 stars 23 forks source link

Remove Sendable requirements in LockIsolated #40

Closed arnauddorgans closed 1 month ago

arnauddorgans commented 1 month ago

I don't see the point of requiring Sendable conformance here, as we can instantiate the LockIsolated without it

let timer: LockIsolated<Timer?> = .init(nil) // OK
timer.setValue(newValue) // OK
timer.value // Conformance of 'Timer' to 'Sendable' is unavailable

In the meantime, I use this workaround

public extension LockIsolated {
    @available(*, deprecated, message: "Waiting for PR")
    var _value: Value {
        self.withValue { UncheckedSendable($0) }.value
    }
}
mbrandonw commented 1 month ago

Hi @arnauddorgans, unfortunately this is not correct to do. The whole point of LockIsolated is to lock all access to the non-sendable value. Once you put a non-sendable thing into the LockIsolated you cannot get it back out because that would let you do things with it that are not concurrency safe.

I am going to close this for now, but if you have more questions you can open a discussion.

jshier commented 1 month ago

By that logic it's not safe to put a non-Sendable value into the lock in the first place, as you could do things with it that aren't concurrency safe.

Perhaps you could split the difference and add API to match at least part of what Mutex does?

borrowing func withLock<Result, E>(_ body: (inout sending Value) throws(E) -> sending Result) throws(E) -> sending Result where E : Error, Result : ~Copyable

Whether that would solve the original motivation I don't know, but it would help capture some of the functionality.

arnauddorgans commented 1 month ago

I see, but the purpose of LockIsolated is not to safe the access using a NSRecursiveLock ? Even for non sendable values?

So why not require

LockIsolated<Value: Sendable>

?

mbrandonw commented 1 month ago

By that logic it's not safe to put a non-Sendable value into the lock in the first place, as you could do things with it that aren't concurrency safe.

@jshier No, the lock forces you to interact with the non-sendable value in a controlled manner. For example:

func testSendable() async {
  class Count {
    var value = 0
    func increment() { value += 1 }
  }

  let count = LockIsolated(Count())

  await withTaskGroup(of: Void.self) { group in
    for _ in 1...10_000 {
      group.addTask {
        count.withValue { $0.increment() }
      }
    }
  }

  XCTAssertEqual(count.withValue { $0.value }, 10_000)
}

If LockIsolated allowed escaping a non-sendable value then you could do this:

//count.withValue { $0.increment() }
count.value.increment()

…and with that change the test fails.

Perhaps you could split the difference and add API to match at least part of what Mutex does?

Yes that is possible, and we have some branches exploring this. It's just not a top priority right now.

mbrandonw commented 1 month ago

So why not require

LockIsolated<Value: Sendable>

?

@arnauddorgans Look at the example I posted above. That shows it is perfectly fine to put a non-sendable thing in LockIsolated. You just can't escape it from the lock.

jshier commented 1 month ago

Yes, I meant something more like

let count = Count()
let lockedCount = LockIsolated(count)
count.increment() // Whoops!

That's just as dangerous as allowing the extraction of the value.

Looks like Mutex fixes this in the init by consuming the value: init(_ initialValue: consuming sending Value).

mbrandonw commented 1 month ago

Yes, newer Swift tools make the guarantees stronger, but that is still no reason to purposely make LockIsolated more dangerous than it needs to be.

arnauddorgans commented 1 month ago

Thanks!