wez / wzsh

Wez's Shell
MIT License
79 stars 5 forks source link

[Question] Should `filedescriptor::OwnedHandle` be `Sync` on Windows #4

Closed quark-zju closed 4 years ago

quark-zju commented 4 years ago

Context: I'm trying to build streampager on Windows. It uses TerminalWaker which uses OwnedHandle, which has a raw pointer and isn't Sync. Then TerminalWaker is neither Send or Sync since it contains Arc<OwnedHandle>, preventing it from being passed across thread.

OwnedHandle is Sync on unix. So maybe it can also be Sync on Windows?

wez commented 4 years ago

Good catch. I think that strictly speaking OwnedHandle probably shouldn't be Sync on any platform, because operating on that handle from multiple threads can result in data races.

That said, I think it would be totally reasonable to add an:

/// Windows defines it as safe to `set` an event handle from any thread
unsafe impl Sync for EventHandle {}

to the termwiz code and I think that will effectively unblock using it in streampager?

wez commented 4 years ago

(OwnedHandle should be Send on all platforms though, looks like that is broken on Windows)

quark-zju commented 4 years ago

strictly speaking OwnedHandle probably shouldn't be Sync on any platform

Perhaps impl !Sync can be added to the unix implementation for consistency?

std::fs::File impls Sync so maybe it's not that bad to be Sync?

unsafe impl Sync for EventHandle {}

Sounds good. I didn't find other blockers so maybe a point release is helpful.

OwnedHandle should be Send on all platforms though

It is Send. But Arc<OwnedHandle> is not Send (otherwise a cloned Arc can be sent to other threads, making a Send type Sync incorrectly).

quark-zju commented 4 years ago

Closing since the original issue was resolved and it seems good to not change the current implementation.