search

YaLTeR / wl-clipboard-rs

A safe Rust crate for working with the Wayland clipboard.
Apache License 2.0
255 stars 17 forks source link

Copy freezes when tried out in wldash #6

Closed kennylevinsen closed 4 years ago

kennylevinsen commented 5 years ago

Testing out wl-clipboard-rs in kennylevinsen/wldash@a3d89aadd5068a95e934aab5d45c0a1234539600, the copy operation simply freezes.

To reproduce:

Note that you may want to have a different TTY ready to kill wldash, or start it with a "cargo run & sleep 10; kill -9 %1", as wldash still has a keyboard grab even if frozen.

YaLTeR commented 5 years ago

Huh. I'll try it out tomorrow. I wonder if it's related to trying to using wayland-rs within an existing wayland-rs thing...

сб, 5 окт. 2019 г., 0:10 Kenny Levinsen notifications@github.com:

Testing out wl-clipboard-rs in kennylevinsen/wldash@a3d89aa https://github.com/kennylevinsen/wldash/commit/a3d89aadd5068a95e934aab5d45c0a1234539600, the copy operation simply freezes.

To reproduce:

  • Build that commit
  • Run wldash
  • Type "=1+1" (the calculator copies on return)
  • See that wldash is frozen

Note that you may want to have a different TTY ready to kill wldash, or start it with a "cargo run & sleep 10; kill -9 %1", as wldash still has a keyboard grab even if frozen.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/YaLTeR/wl-clipboard-rs/issues/6?email_source=notifications&email_token=AANWCVG5EH5UOVDIFC3NERTQM6WKTA5CNFSM4I5UOTW2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HPY2GLQ, or mute the thread https://github.com/notifications/unsubscribe-auth/AANWCVAQ65CTRA54OVRFUGTQM6WKTANCNFSM4I5UOTWQ .

YaLTeR commented 5 years ago

...oh. So, it doesn't actually freeze, in fact it works correctly: it forks (as per the default setting) and then serves copy requests, while the parent wldash exits fine... But the fork continues to have the wldash Wayland surface displayed. You can verify that it works correctly by trying to wl-paste from another TTY, and then by trying to wl-copy, so the wldash selection is cleared, and then it exits.

I'm not quite sure if it's possible to fork cleanly from the wl-clipboard-rs side. Have you got any ideas?

kennylevinsen commented 5 years ago

Yikes. Hmm. That's a problem.

For a single-threaded application, the only way to deal with this would be to manually close the existing wayland fd in the child, which would require the library user themselves to issue the fork.

However, note the emphasis. wldash is, for annoying reasons, multi-threaded, and forked children in multi-threaded applications must only call async-safe functions. That excludes things like malloc(3). As a rule of thumb, the only safe thing to do is pretty much just to exec.

YaLTeR commented 5 years ago

I think forking in wl-clipboard-rs should be removed then (or marked as unsafe, but I'd like to keep this crate safe Rust-only as far as reasonably possible). I should be able to move the fork into wl-copy itself (where it is required). Then applications could either blocking-copy on a separate thread, manage the fork themselves, or just call wl-copy I guess.

bugaevc commented 5 years ago

or just call wl-copy I guess

And we're back to square 1 :wink:

YaLTeR commented 4 years ago

I pushed a branch, no-fork, which removes forking from the crate code and replaces it with a regular thread. It also adds a helper function to accommodate the previous forking workflow that e.g. wl-copy depended on (see the wl-copy change).

I intend to dogfood the binaries for a little and if all is fine push that as a new release, 0.4.0, where the API is mostly unchanged.

Further API developments (#11) will happen past that. It's important to get a version with this fix out because currently wl-clipboard-rs is essentially unsafe in multi-threaded programs.

YaLTeR commented 4 years ago

Those changes are now in master, split into more commits.

YaLTeR commented 4 years ago

@yory8 could you please check if the new thread-spawning copying works fine for your clipboard manager use-case?

@kennylevinsen I suppose for the wldash use-case you want the forking behavior after all; I'm not sure if it's worth forking from wldash itself rather than just calling wl-copy as is currently done. It could me more trouble than it's worth (especially if you use threads).

ghost commented 4 years ago

@YaLTeR I'm having trouble upgrading to the new API because I have a vector, whereas now Source wants an array to put into Box<[u8]>. Besides that, I don't think it will give me problems.

YaLTeR commented 4 years ago

You can convert a vector to a Box<[u8]> with into_boxed_slice.

kennylevinsen commented 4 years ago

@kennylevinsen I suppose for the wldash use-case you want the forking behavior after all; I'm not sure if it's worth forking from wldash itself rather than just calling wl-copy as is currently done. It could me more trouble than it's worth (especially if you use threads).

In the non-daemonizing mode, yeah, I need forking behavior for it to the copy to survive wldash, with fork either done pre-threads or fork/exec to an independent process (such as wl-copy). I agree that it is simplest to just stick to wl-copy.

In daemonizing mode, I could technically have a thread as long as it does not interfere with the main wayland connection, but having two implementations is a bit silly.

YaLTeR commented 4 years ago

I published a v0.4.0, so I'm closing this issue now.