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

eval "man -k ." | fzf | read hangs on mac #6806

Closed
introom opened this issue Mar 25, 2020 · 13 comments
Closed

eval "man -k ." | fzf | read hangs on mac #6806

introom opened this issue Mar 25, 2020 · 13 comments
Milestone

Comments

@introom
Copy link

introom commented Mar 25, 2020

eval "man -k ." | fzf | read
fish version 3.1.0
macos

Used to work fine before 3.1.0

@krobelus
Copy link
Member

krobelus commented Mar 25, 2020

I can reproduce when running fish interactively, it seems like first read takes control of the terminal.
If you press Control-C after some time, the fzf UI becomes visible.

Happens since 2fe2169 which made eval a builtin because fish blocks when executing builtins.

This workaround seems to restore the old behavior:

begin; eval "man -k ."; end | fzf | read

(or man -k . | fzf | read)

@krobelus krobelus added this to the fish-future milestone Mar 25, 2020
@zanchey
Copy link
Member

zanchey commented Apr 16, 2020

@ridiculousfish @mqudsi any thoughts? If the workaround is enough, I don't think this needs to be fixed for 3.1.*, though of course it would be nice.

@ridiculousfish
Copy link
Member

ridiculousfish commented Apr 25, 2020

I thought this would be the same as #6624 but it doesn't appear to be.

@ridiculousfish
Copy link
Member

ridiculousfish commented Apr 25, 2020

This ends up being similar to #6624 except we need to thread the pgid into eval as well.

@ridiculousfish
Copy link
Member

ridiculousfish commented Apr 26, 2020

So the problems here are:

  1. Commands spawned by eval would run in a different pgroup. which would fight with fzf for control of the terminal. So we have to tell eval to propagate the pgroup.
  2. eval would just propagate the io_chain, but this might contain a pipe that is intended to be used by fzf, which is not yet launched. So we would deadlock. The fix is for eval to direct its output to the provided buffers, like other builtins..

This points the way for a lot of cleanup - output_stream_t is "the thing that builtins write to" and should be unified with io_data_t. Also pgroup management continues to vex but I have a plan for that.

I think this should go into a patch release, I will prepare a commit.

ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this issue Apr 26, 2020
Prior to this fix, builtin_eval would direct output to the io_chain of the
job. The problem is with pipes: `builtin_eval` might happily attempt to
write unlimited output to the write end of a pipe, but the corresponding
reading process has not yet been launched. This results in deadlock.

The fix is to buffer all the output from `builtin_eval`. This is not fun
but the best that can be done until we have real concurrent processes.

Fixes fish-shell#6806
ridiculousfish added a commit that referenced this issue Apr 26, 2020
Ensure that if eval is invoked as part of a pipeline, any jobs spawned
by eval will have the same pgroup as the parent job.

cherry-pick of 82f2d86

Partially fixes #6806
ridiculousfish added a commit that referenced this issue Apr 26, 2020
Prior to this fix, builtin_eval would direct output to the io_chain of the
job. The problem is with pipes: `builtin_eval` might happily attempt to
write unlimited output to the write end of a pipe, but the corresponding
reading process has not yet been launched. This results in deadlock.

The fix is to buffer all the output from `builtin_eval`. This is not fun
but the best that can be done until we have real concurrent processes.

cherry-pick of a1f1b9c

Fixes #6806
@ridiculousfish
Copy link
Member

ridiculousfish commented Apr 26, 2020

I merged this into 3.1.1 as 5fccfd8

@novadev94
Copy link

novadev94 commented Apr 27, 2020

I'm having some trouble with the newest change, all "eval" commands here hang up: https://github.com/junegunn/fzf/blob/master/shell/key-bindings.fish

Can reproduce it with a simple command like eval vim foo.txt, it works fine with revision 389c5e7

@krobelus
Copy link
Member

krobelus commented Apr 27, 2020

Probably the non-buffering behavior can safely be restored for streams that are not redirected:

diff --git a/src/builtin_eval.cpp b/src/builtin_eval.cpp
index c9495a669..52b33d2f4 100644
--- a/src/builtin_eval.cpp
+++ b/src/builtin_eval.cpp
@@ -27,22 +27,34 @@ int builtin_eval(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
         new_cmd += argv[i];
     }
 
+    shared_ptr<io_bufferfill_t> stdout_fill, stderr_fill;
+    io_chain_t io = *streams.io_chain;
     // The output and errput of eval must go to our streams, not to the io_chain in our streams,
-    // because they may contain a pipe which is intended to be consumed by a process which is not
-    // yet launched (#6806). Make bufferfills to capture it.
-    // TODO: we are sloppy and do not honor other redirections; eval'd code won't see for example a
-    // redirection of fd 3.
-    shared_ptr<io_bufferfill_t> stdout_fill =
-        io_bufferfill_t::create(fd_set_t{}, parser.libdata().read_limit, STDOUT_FILENO);
-    shared_ptr<io_bufferfill_t> stderr_fill =
-        io_bufferfill_t::create(fd_set_t{}, parser.libdata().read_limit, STDERR_FILENO);
-    if (!stdout_fill || !stderr_fill) {
-        // This can happen if we are unable to create a pipe.
-        return STATUS_CMD_ERROR;
+    // because they may contain a pipe which is intended to be consumed by a process which is
+    // not yet launched (#6806). Make bufferfills to capture it.
+    // TODO: we are sloppy and do not honor other redirections; eval'd code won't see for
+    // example a redirection of fd 3.
+    if (streams.out_is_redirected) {
+        stdout_fill =
+            io_bufferfill_t::create(fd_set_t{}, parser.libdata().read_limit, STDOUT_FILENO);
+        if (!stdout_fill) {
+            // This can happen if we are unable to create a pipe.
+            return STATUS_CMD_ERROR;
+        }
+        io.push_back(stdout_fill);
+    }
+    if (streams.err_is_redirected) {
+        stderr_fill =
+            io_bufferfill_t::create(fd_set_t{}, parser.libdata().read_limit, STDERR_FILENO);
+        if (!stderr_fill) {
+            // This can happen if we are unable to create a pipe.
+            return STATUS_CMD_ERROR;
+        }
+        io.push_back(stderr_fill);
     }
 
     int status = STATUS_CMD_OK;
-    auto res = parser.eval(new_cmd, io_chain_t{stdout_fill, stderr_fill}, streams.parent_pgid);
+    auto res = parser.eval(new_cmd, io, streams.parent_pgid);
     if (res.was_empty) {
         // Issue #5692, in particular, to catch `eval ""`, `eval "begin; end;"`, etc.
         // where we have an argument but nothing is executed.
@@ -54,12 +66,15 @@ int builtin_eval(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
     // Finish the bufferfills - exhaust and close our pipes.
     // Note it is important that we hold no other references to the bufferfills here - they need to
     // deallocate to close.
-    std::shared_ptr<io_buffer_t> output = io_bufferfill_t::finish(std::move(stdout_fill));
-    std::shared_ptr<io_buffer_t> errput = io_bufferfill_t::finish(std::move(stderr_fill));
+    if (stdout_fill) {
+        std::shared_ptr<io_buffer_t> output = io_bufferfill_t::finish(std::move(stdout_fill));
+        streams.out.append_narrow_buffer(output->buffer());
+    }
 
-    // Copy the output from the bufferfill back to the streams.
-    streams.out.append_narrow_buffer(output->buffer());
-    streams.err.append_narrow_buffer(errput->buffer());
+    if (stderr_fill) {
+        std::shared_ptr<io_buffer_t> errput = io_bufferfill_t::finish(std::move(stderr_fill));
+        streams.err.append_narrow_buffer(errput->buffer());
+    }
 
     return status;
 }

ridiculousfish added a commit that referenced this issue Apr 28, 2020
Commit 5fccfd8, with the fix for #6806,
switched eval to buffer its output (like other builtins do). But this
prevents using eval with commands that wants to see the tty, especially
fzf. So only buffer the output if the output is piped to the next process.

This will solve #6955 (which needs to go into a point release).
@max-sixty
Copy link

max-sixty commented Apr 28, 2020

FYI having upgraded to 3.1.1, my fzf shortcuts now cause the shell to hang. Is this related? Lmk if I can provide any info to make resolving this easier. Thanks. (and thanks for such an awesome shell!)

ridiculousfish added a commit that referenced this issue Apr 28, 2020
Commit 5fccfd8, with the fix for #6806,
switched eval to buffer its output (like other builtins do). But this
prevents using eval with commands that wants to see the tty, especially
fzf. So only buffer the output if the output is piped to the next process.
@ridiculousfish
Copy link
Member

ridiculousfish commented Apr 28, 2020

@krobelus you were exactly right, I wish I had spotted your review before the 3.1.1 release! #6955 is tracking the regression I introduced. @max-sixty, that's what you're encountering as well.

ridiculousfish added a commit that referenced this issue Apr 28, 2020
@max-sixty
Copy link

max-sixty commented Apr 28, 2020

Thanks @ridiculousfish & @krobelus !

@krobelus
Copy link
Member

krobelus commented Apr 28, 2020

@ridiculousfish Sadly I didn't see it either until after the release when the reports came in.

@novadev94
Copy link

novadev94 commented Apr 28, 2020

Just tested out the latest commit on master, things now work like a charm! Thanks to all you guys!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants