martanne / vis

A vi-like editor based on Plan 9's structural regular expressions
Other
4.21k stars 258 forks source link

vis_pipe() doesn't correctly handle programs that fork to the background #929

Open caioraposo opened 3 years ago

caioraposo commented 3 years ago

Using wl-copy results in a weird behavior where it keeps receiving text through the command prompt. Tested with neovim and acme, it works as expected. wl-paste works.

vis: v0.7 wlroots: v0.12 sway: v1.15

2021-02-10T14:22:15,566869047-03:00

martanne commented 3 years ago

Thanks for the report.

I briefly looked into the issue and the problem is that we don't actually wait for process termination, but until all connected pipes to the standard I/O streams are closed. However, wl-copy forks to the background, but doesn't close its standard error file descriptor which means we keep waiting for it.

This can be reproduced with the following shell pipe line which does not terminate.

echo "text to copy" | wl-copy 2>&1 >/dev/null | cat

A similar problem was recently fixed for the standard input and output descriptors in a wl-copy commit. If you apply the same changes for standard error, it should work.

It could also be argued that we should close our end of the pipe once we notice the exit of the original process.

caioraposo commented 3 years ago

Sorry for being a little late, but thanks for the response. Do you think we should handle possible error messages from stderr as suggested by bugaevc?

mcepl commented 1 year ago

@ninewise, @caioraposo Is this still relevant, wasn’t this already fixed?

caioraposo commented 1 year ago

@mcepl I still have this issue so I put the following in my visrc:

vis:map(vis.modes.NORMAL, 'Y', ':> wl-copy 2>/dev/null -n<Enter>')
mcepl commented 1 year ago

@mcepl I still have this issue so I put the following in my visrc:

  1. What is your version of vis (or which commit is your vis based on)?
  2. Describe exact steps how you reproduce the issue and what exactly happens when you do it?
rnpnr commented 1 year ago

I'm closing this issue since I believe this should have been fixed by 0cccd6e.

If its still an issue it can be reopened along with the exact steps needed to recreate it.

fischerling commented 1 year ago

I can still reproduce the hanging wl-copy process with the current master.

Steps to reproduce

  1. Build vis on current master ./configure && make
  2. Open vis ./vis
  3. Write some text
  4. Select all written lines
  5. Run :> wl-copy. The wl-copy command does not terminate
  6. Terminate the wl-copy process by hitting Ctrl+C. The selection is NOT copied to the clipboard
  7. run :> wl-copy 2>/dev/null
  8. The selection is properly copied and wl-copy does terminate``
bugaevc commented 1 year ago

Hello,

repeating what I've said in https://github.com/bugaevc/wl-clipboard/pull/110:

In case of an editor using wl-copy internally to implement copying, the proper fix would be for the editor to consider the text copied once the parent wl-copy exits, but keep watching for potential errors (and perhaps displaying them to the user) until the pipe is closed. Note that wl-clipboard is not the only clipboard program to use background-forking, e.g. xclip does the same, so the fix wouldn't specifically target wl-clipboard either.

Redirecting 2>/dev/null works, but it's a workaround that hides any error messages that wl-copy may output.

rnpnr commented 1 year ago

I see, this makes more sense now. Right now vis-clipboard redirects xclip's stderr to /dev/null (see here). I'm not saying its the best solution but for the meantime I think doing the same for wl-copy would be sufficient. Thoughts?

mcepl commented 5 months ago

Still cannot reproduce: :>wl-copy just nicely finish and text is copied (vis from commit d3e4af1f and wl-copy 2.2.0).

fischerling commented 5 months ago

For me your example unfortunately still hangs even. But I haven't investigated further. The wl-clipboard issue tracker has a lot of open issues, which seem related.

apprehensions commented 4 months ago

I can still reproduce ( vis 004800e348cba1bf5f4536876b4e0eafbcf1ea58 wl-clipboard 2.2.0), and adding 2>/dev/null fixes the problem as well .