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

Abort when running multiple read in a row on Fish 2.6.0 #4206

Closed
0rax opened this Issue Jul 10, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@0rax

0rax commented Jul 10, 2017

Description of the bug:

When running multiple read in a row without creating a full interactive session, fish will abort.

This can be reproduced by launching:

fish -c 'read a; read b'

or executing a script such as:

#!/usr/bin/env fish
read a
read b

Note that this issue is also seen when running it as:

fish -i -c 'read a; read b'

This produce the following output:

<E> fish: src/reader.cpp:1559: failed assertion: input_initialized
[ ... ]
Aborted

The complete output of my test can be seen here: https://gist.github.com/0rax/d7392b40f61c6081a12ccc93c664d3eb

This was not present in Fish v2.5 and has been tested on ArchLinux + Fish v2.5.0.

Bug seen on:

  • Fish 2.6.0 (via brew install fish)
    • macOS Sierra + Iterm2
    • macOS Sierra + Terminal.app
  • Fish 2.6.0 (installed via apk add fish@edge)
    • AlpineLinux + Iterm2 via SSH
  • Fish 2.6.0-208-gc577d012 (via brew install --build-from-source fish --HEAD)
    • macOS Sierra + Iterm2
    • macOS Sierra + Terminal.app
@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Jul 11, 2017

Member

The error here is:

<E> fish: src/reader.cpp:1542: failed assertion: input_initialized

which bisects to 6d02bec.

@krader1961, any ideas?

Member

zanchey commented Jul 11, 2017

The error here is:

<E> fish: src/reader.cpp:1542: failed assertion: input_initialized

which bisects to 6d02bec.

@krader1961, any ideas?

@zanchey zanchey added the bug label Jul 11, 2017

@zanchey zanchey added this to the fish 2.7.0 milestone Jul 11, 2017

@krader1961 krader1961 self-assigned this Jul 16, 2017

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jul 16, 2017

Contributor

I had replaced an init_input() call with an assert because my refactoring as part of fixing a different problem suggested that call was no longer needed. And no unit tests failed and the resulting binary worked for me. The code in question needs considerably more refactoring and renaming of some of the functions to improve clarity. But for the moment I've conditionally reintroduced the init_input() call and added a unit test to keep this from regressing in the future.

Contributor

krader1961 commented Jul 16, 2017

I had replaced an init_input() call with an assert because my refactoring as part of fixing a different problem suggested that call was no longer needed. And no unit tests failed and the resulting binary worked for me. The code in question needs considerably more refactoring and renaming of some of the functions to improve clarity. But for the moment I've conditionally reintroduced the init_input() call and added a unit test to keep this from regressing in the future.

@0rax

This comment has been minimized.

Show comment
Hide comment
@0rax

0rax Jul 16, 2017

Your commit seems to have fix the issue for simple double calls but I still experience some issue with read in some context.

I first discovered this issue when running complex read based scripts such as fishline/tests/run.fish. It seems that with this fix, it now crashes after the first call to read in __fishline_test and no longer the second time this function is called.

This test script creates 2 functions and pass them as arguments to read for each subsection we want to test, we are doing so with:

function func_right
    [...]
end

function func_left
    [...]
end

read -R func_right -p func_left

I've done some testing to pin down the issue, and I saw that it now segv when calling read with with either -P, -R or -p with no variable to assign to.

For example running read -P ">" a would work while read -P ">" will segfault.

You can see my full test logs here: https://gist.github.com/0rax/2ca4b8029209d7165274a5097d9bbac8.

To go back to the context of __fishline_test function, applying the following patch fixes the issue:

diff --git a/internals/__fishline_test.fish b/internals/__fishline_test.fish
index f53b2f7..f12654a 100644
--- a/internals/__fishline_test.fish
+++ b/internals/__fishline_test.fish
@@ -16,8 +16,9 @@ function __fishline_test --argument-names segment -d 'test segments'
         fishline -l -s $FLINT_TEST_STATUS $FLINT_TEST_SEG
     end

-    read -R flint_test_right -p flint_test_left
+    read -R flint_test_right -p flint_test_left TEST_VAR

+    set -e FLINT_TEST_VAR
     set -e FLINT_TEST_SEG
     set -e FLINT_TEST_STATUS

It looks like this fix (5dc78dd) introduced a new regression while fixing one.

FYI : The test script I found these issues with had been working fine since it was created (since Fish 2.4.0 IIRC).

0rax commented Jul 16, 2017

Your commit seems to have fix the issue for simple double calls but I still experience some issue with read in some context.

I first discovered this issue when running complex read based scripts such as fishline/tests/run.fish. It seems that with this fix, it now crashes after the first call to read in __fishline_test and no longer the second time this function is called.

This test script creates 2 functions and pass them as arguments to read for each subsection we want to test, we are doing so with:

function func_right
    [...]
end

function func_left
    [...]
end

read -R func_right -p func_left

I've done some testing to pin down the issue, and I saw that it now segv when calling read with with either -P, -R or -p with no variable to assign to.

For example running read -P ">" a would work while read -P ">" will segfault.

You can see my full test logs here: https://gist.github.com/0rax/2ca4b8029209d7165274a5097d9bbac8.

To go back to the context of __fishline_test function, applying the following patch fixes the issue:

diff --git a/internals/__fishline_test.fish b/internals/__fishline_test.fish
index f53b2f7..f12654a 100644
--- a/internals/__fishline_test.fish
+++ b/internals/__fishline_test.fish
@@ -16,8 +16,9 @@ function __fishline_test --argument-names segment -d 'test segments'
         fishline -l -s $FLINT_TEST_STATUS $FLINT_TEST_SEG
     end

-    read -R flint_test_right -p flint_test_left
+    read -R flint_test_right -p flint_test_left TEST_VAR

+    set -e FLINT_TEST_VAR
     set -e FLINT_TEST_SEG
     set -e FLINT_TEST_STATUS

It looks like this fix (5dc78dd) introduced a new regression while fixing one.

FYI : The test script I found these issues with had been working fine since it was created (since Fish 2.4.0 IIRC).

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jul 17, 2017

Contributor

Thanks for the second failure report, @0rax. That problem, however, is unrelated to the original problem or commit 5dc78dd. The second problem is due to commit d383e3b. I'll open a new issue for that problem.

Contributor

krader1961 commented Jul 17, 2017

Thanks for the second failure report, @0rax. That problem, however, is unrelated to the original problem or commit 5dc78dd. The second problem is due to commit d383e3b. I'll open a new issue for that problem.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jul 17, 2017

Contributor

@0rax, What did you expect to happen when you do read with no var name(s)?

Contributor

krader1961 commented Jul 17, 2017

@0rax, What did you expect to happen when you do read with no var name(s)?

@0rax

This comment has been minimized.

Show comment
Hide comment
@0rax

0rax Jul 17, 2017

@krader1961 oh right, it just went to the next issue then, just telling that your last commit uncovered it !

I am using it pretty much as a way to preview a prompt, this making it pretty much a side-effect (just print and ignore input), so when called without any variable name after I am expecting that either no variable is assigned or as for bash's read a default variable such as REPLY might be filled that I will ignore afterwards.

Though as seen in your new issue, I am completely fine with forcing a variable name to be set when calling read if it's what follow fish's philosophy. This will make a script using read backward compatible but will still be a forward breaking change for older calls.

In my case, I will push my change to the repository to make sure a variable is set even though I am not using it.

0rax commented Jul 17, 2017

@krader1961 oh right, it just went to the next issue then, just telling that your last commit uncovered it !

I am using it pretty much as a way to preview a prompt, this making it pretty much a side-effect (just print and ignore input), so when called without any variable name after I am expecting that either no variable is assigned or as for bash's read a default variable such as REPLY might be filled that I will ignore afterwards.

Though as seen in your new issue, I am completely fine with forcing a variable name to be set when calling read if it's what follow fish's philosophy. This will make a script using read backward compatible but will still be a forward breaking change for older calls.

In my case, I will push my change to the repository to make sure a variable is set even though I am not using it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment