magic-wormhole / magic-wormhole.rs

Rust implementation of Magic Wormhole, with new features and enhancements
European Union Public License 1.2
645 stars 72 forks source link

add timeout to clipboard initialization #185

Closed jonZlotnik closed 1 year ago

jonZlotnik commented 1 year ago

TL;DR

Context: I wanted to try my hand at the sending text feature, so I tried getting this repo setup on WSL2, but I couldn't get my binary to run. It kept hanging with no output.

Into debug mode I went: All I got was [2023-03-06T23:58:31Z DEBUG wormhole_rs] Logging enabled.

Now to debug Rust for the first time: I found that it was stuck inside the cli_clipboard ClipboardContext::new() function. And noticed that for Linux environments, it depends on an x11 socket.

image

There doesn't seem to be any kind of test suite for the cli at the moment, so let me know if there something I should add.

Note: I'm brand spanking new to Rust. So please be verbose with comments and teachings. πŸ˜…

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage has no change and project coverage change: -0.17 :warning:

Comparison is base (a82a7ae) 43.27% compared to head (79ebb44) 43.10%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #185 +/- ## ========================================== - Coverage 43.27% 43.10% -0.17% ========================================== Files 18 18 Lines 2542 2552 +10 ========================================== Hits 1100 1100 - Misses 1442 1452 +10 ``` | [Impacted Files](https://codecov.io/gh/magic-wormhole/magic-wormhole.rs/pull/185?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole) | Coverage Ξ” | | |---|---|---| | [cli/src/main.rs](https://codecov.io/gh/magic-wormhole/magic-wormhole.rs/pull/185?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole#diff-Y2xpL3NyYy9tYWluLnJz) | `0.00% <0.00%> (ΓΈ)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

jonZlotnik commented 1 year ago

Note: I'm actually having trouble replicating the issue I faced in other environments (even other WSL2 envs). Nonetheless, blocking the main thread of any program on waiting for an "networky" OS resource without a timeout is usually not a good idea.

piegamesde commented 1 year ago

I'll try to have a look at it in the next couple of days. In the mean time, would you mind reporting the bug upstream to the relevant crate? Maybe they have some deadlock or other bug that can be fixed for that platform.

jonZlotnik commented 1 year ago

Issues seem to already be stacking up. Maybe switching to 1Password's arboard https://github.com/1Password/arboard could be good if they're not resolved timely?

cli_clipboard https://github.com/ActuallyAllie/cli-clipboard/issues/16 https://github.com/ActuallyAllie/cli-clipboard/issues/17

x11_clipboard https://github.com/quininer/x11-clipboard/issues/32

@piegamesde you also added this and it's still open https://github.com/ActuallyAllie/cli-clipboard/issues/10

piegamesde commented 1 year ago

Thanks for digging into this. I'm fine if you want to make a switch to arboard, the only requirement I have is that it has native Wayland support. Using it would have the added benefit of not depending on any native X libraries, as we currently do.

piegamesde commented 1 year ago

Finally had a look at the code. It looks okay, but I think it can be simplified: since we always only send one value, you can simply have it as return value of the closure and not use a channel at all. On the main thread, one would then join the clipboard thread with a timeout. If it succeeds print the value, otherwise kill the thread and print an error message.

Of course you could also try out arboard, which will probably make the background thread obsolete.

jonZlotnik commented 1 year ago

Okay, so I think you're correct in that the timeout fix should be upstream, but the cli_clipoard is too far downstream (depends on x11_clipboard, which depends on x11rb).

So I tried switching to arboard, but it actually has the same issue. Thankfully, they depend on x11rb directly, and a small fix is possible. So I submitted a PR there, and we'll see how it progresses.

https://github.com/1Password/arboard/pull/100

If it goes through, I'll PR the switch to arboard.

jonZlotnik commented 1 year ago

Finally had a look at the code. It looks okay, but I think it can be simplified: since we always only send one value, you can simply have it as return value of the closure and not use a channel at all. On the main thread, one would then join the clipboard thread with a timeout. If it succeeds print the value, otherwise kill the thread and print an error message.

Of course you could also try out arboard, which will probably make the background thread obsolete.

I actually don't think it's possible without a channel unless there's some API I missed for joining threads. (very well might be) Don't want to wait the full time if the thread returns successfully.

jonZlotnik commented 1 year ago

Thanks for digging into this. I'm fine if you want to make a switch to arboard, the only requirement I have is that it has native Wayland support. Using it would have the added benefit of not depending on any native X libraries, as we currently do.

Alright, so my PR finally got merged in https://github.com/1Password/arboard/pull/100

There seems to be the option to have Wayland by default: https://github.com/1Password/arboard/blob/3399a2582f33085f9602b1acfaaa5847face8373/README.md?plain=1#L21-L27

piegamesde commented 1 year ago

Okay, how to proceed with this? Do you want to try switching to arboard?

jonZlotnik commented 1 year ago

Okay, how to proceed with this? Do you want to try switching to arboard?

Yeah I'll PR the switch to arboard. Sorry for the lapse.

jonZlotnik commented 1 year ago

Switch is PR'd in https://github.com/magic-wormhole/magic-wormhole.rs/pull/203