Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
caioraposo opened this issue Feb 10, 2021 · 12 comments
Open

Comments

@caioraposo
Copy link

caioraposo commented Feb 10, 2021

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 added a commit that referenced this issue Feb 15, 2021
Once we have written all data we should properly close the (correct)
pipe. Before we wrongly closed the pipe connected to the standard output
stream.

More generally, we currently do not listen for child process termination,
but instead wait until all the connected pipes are closed. This might
be problematic in case the external process keeps hold of the standard
I/O file descriptors. One particular example of this is wl-copy(1).

See #929
@martanne
Copy link
Owner

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
Copy link
Author

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
Copy link
Contributor

mcepl commented Nov 29, 2022

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

@caioraposo
Copy link
Author

@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
Copy link
Contributor

mcepl commented Dec 24, 2022

@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?

erf pushed a commit to erf/vis that referenced this issue Dec 30, 2022
Once we have written all data we should properly close the (correct)
pipe. Before we wrongly closed the pipe connected to the standard output
stream.

More generally, we currently do not listen for child process termination,
but instead wait until all the connected pipes are closed. This might
be problematic in case the external process keeps hold of the standard
I/O file descriptors. One particular example of this is wl-copy(1).

See martanne#929
@rnpnr
Copy link
Collaborator

rnpnr commented May 31, 2023

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.

@rnpnr rnpnr closed this as completed May 31, 2023
@fischerling
Copy link
Contributor

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

  • wl-clipboard version 2.1.0
  • sway version 1.8.1

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
Copy link

bugaevc commented Jun 5, 2023

Hello,

repeating what I've said in bugaevc/wl-clipboard#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
Copy link
Collaborator

rnpnr commented Jun 5, 2023

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?

@rnpnr rnpnr reopened this Jun 5, 2023
@rnpnr rnpnr changed the title Can't copy with wl-copy vis_pipe() doesn't correctly handle programs that fork to the background Jun 6, 2023
@mcepl
Copy link
Contributor

mcepl commented Feb 14, 2024

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

@fischerling
Copy link
Contributor

fischerling commented Feb 15, 2024 via email

@apprehensions
Copy link
Contributor

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

git-bruh added a commit to git-bruh/vis that referenced this issue Apr 6, 2024
git-bruh added a commit to git-bruh/vis that referenced this issue May 3, 2024
git-bruh added a commit to git-bruh/vis that referenced this issue May 7, 2024
jeremybobbin pushed a commit to jeremybobbin/vis that referenced this issue Aug 17, 2024
Once we have written all data we should properly close the (correct)
pipe. Before we wrongly closed the pipe connected to the standard output
stream.

More generally, we currently do not listen for child process termination,
but instead wait until all the connected pipes are closed. This might
be problematic in case the external process keeps hold of the standard
I/O file descriptors. One particular example of this is wl-copy(1).

See martanne#929
jeremybobbin pushed a commit to jeremybobbin/vis that referenced this issue Nov 24, 2024
Once we have written all data we should properly close the (correct)
pipe. Before we wrongly closed the pipe connected to the standard output
stream.

More generally, we currently do not listen for child process termination,
but instead wait until all the connected pipes are closed. This might
be problematic in case the external process keeps hold of the standard
I/O file descriptors. One particular example of this is wl-copy(1).

See martanne#929
rnpnr added a commit that referenced this issue Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants