input_readch 100% CPU usage #3214

Closed
dewyatt opened this Issue Jul 9, 2016 · 21 comments

Projects

None yet

5 participants

@dewyatt
dewyatt commented Jul 9, 2016

Fish uses 100% CPU under certain circumstances when exiting the graphical terminal it is running in (see other conditions below).

I've been able to confirm that this block of code here is being executed nonstop in this scenario: https://github.com/fish-shell/fish-shell/blob/3ca5ca77fa44d5ad626f24ceafe393e210c263cd/src/input.cpp#L604

I haven't had time to trace things any further than that. Here's what the call stack typically looks like:

(gdb) bt
#0  0x00007f404e9b6fc0 in wcslen () from /usr/lib/libc.so.6
#1  0x00007f404f311b7c in std::char_traits<wchar_t>::length (__s=0x5fe320 L"complete")
    at /build/gcc-multilib/src/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/char_traits.h:358
#2  std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >::compare (
    this=0x24d9290, __s=0x5fe320 L"complete")
    at /build/gcc-multilib/src/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:1394
#3  0x00000000004fce0c in std::operator==<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> > (
    __lhs=L"self-insert", __rhs=0x5fe320 L"complete") at /usr/include/c++/6.1.1/bits/basic_string.h:5086
#4  0x000000000056d732 in input_function_get_code (name=L"self-insert") at src/input.cpp:893
#5  0x000000000056bad3 in input_mapping_execute (m=..., allow_commands=true) at src/input.cpp:454
#6  0x000000000056be3e in input_mapping_execute_matching_or_generic (allow_commands=true) at src/input.cpp:539
#7  0x000000000056c079 in input_readch (allow_commands=true) at src/input.cpp:605
#8  0x00000000005b0bd0 in reader_readline (nchars=0) at src/reader.cpp:2438
#9  0x00000000005b05fa in read_i () at src/reader.cpp:2303
#10 0x00000000005b3c38 in reader_read (fd=0, io=...) at src/reader.cpp:3406
#11 0x00000000005c7fe5 in main (argc=1, argv=0x7ffee30e2a78) at src/fish.cpp:503

fish version installed: 2.3.1 and 3ca5ca7
OS/terminal used: Arch/urxvt

Reproduction steps

Hard to explain? Link to a screen recording

  1. Launch fish within urxvt
  2. su -l (root's shell is also fish - not sure why this is necessary to repro, but it is for me)
  3. close urxvt

Expected results

Fish should…not use 100% CPU.

Actual results

100% CPU usage.

@krader1961 krader1961 added the bug label Jul 9, 2016
@krader1961 krader1961 added this to the fish-future milestone Jul 9, 2016
@krader1961
Member

@dewyatt, This might be the best bug report I've seen since I started working on this project last year. I wasn't able to reproduce the problem on macOS or Ubuntu. Do you still see the spinning process if you do

exec fish -d5 2>/tmp/fish.dbg

after the su -l then close the terminal window? If so you'll obviously want to kill the spinning shell ASAP. Then upload the /tmp/fish.dbg file.

@floam
Member
floam commented Jul 9, 2016 edited

I've gotten similar when fish has continued to run unexpectedly when it doesn't have a terminal. On OS X not running as root.

Sample of fish.txt

@floam
Member
floam commented Jul 9, 2016 edited

screenshot 2016-07-08 at 10 18 04 pm

... But I also can reproduce after changing root's shell on OS X. Yeehaw! Some more info forthcoming.

@floam
Member
floam commented Jul 9, 2016 edited

Terminal.app, fish 2.3.1. I'll mention this happens to be encountered on a OS X 10.12 seed as @krader1961 can't reproduce on OS X 10.11.

fish.dbg.txt

@floam
Member
floam commented Jul 9, 2016 edited

fish_2016-07-08_223145_ikhJ.sample.txt

It doesn't have a stdin/stdout which likely is no surprise - no tty.

-----------------
Process: 12251  Name: fish              Parent: 12250   Status: runnable
Flags:   64-bit,called exec,has controlling terminal,App,Donor

Extmods: Task shows no signs of external modification or tampering
Code signing: No flags! (Not code signed *and* allowed execution)


UID:        0   RUID:     0 SVUID:     0
GID:        0   RGID:     0 SVGID:     0

Virtual size:       2404M (2521010176)  Resident size:        3112K (3186688)
Time:  15:00.61   = 12:44.69 (User)    +  2:15.91 (System)
Syscalls:     192279699 Mach Traps:     166
Disk I/O:          0K Read            4K Written
No Network I/O detected for this process

#Threads:   1/1   (Process has no workqueues)

Can't get Stack Snapshot for this process: Operation not supported

[irrelevant VM data removed]

Process Hierarchy:
 12251 fish  has no children

7 File descriptors: (can't get information or fd is revoked)
(can't get information or fd is revoked)
fish             12251 FD  2w  /private/tmp/fish.dbg @0x1ce29    
fish             12251 FD  3r  pipe 
fish             12251 FD  4w  pipe 
fish             12251 FD  5r  pipe 
fish             12251 FD  6w  pipe 
@floam
Member
floam commented Jul 9, 2016

"has controlling terminal"

@dewyatt
dewyatt commented Jul 9, 2016 edited

Thanks @krader1961 I try to be thorough.

I'm getting a repeating "Unknown keybinding 1" in my log (truncated).

Interestingly I wasn't able to reproduce this in a fresh VM install, though it's still persistent on my system.
I'll attempt to reproduce this in a few weeks on a fresh install when I have some time.

fish.dbg.txt

@krader1961
Member

One possibility is because the can_read() function in src/reader.cpp returns true even if the tty will return EOF; i.e., it is readable but won't return a character when read from. The read-ahead logic in reader_readline() looks suspicious in this regard. There's also some confusing logic that maps WEOF (usually -1) to R_EOF (\uF701).

If I could reproduce this I'm pretty confident I would get to the root cause of the issue fairly quickly. I've tried multiple times on macOS 10.11 (El Capitan) and Ubuntu 16.04 without success. That the stack trace you captured, @dewyatt, includes

#7  0x000000000056c079 in input_readch (allow_commands=true) at src/input.cpp:605

says that input_common_readch() isn't returning R_EOF when the tty has gone away. Bingo! It's returning WEOF which appears to be incorrect since the callers don't check for WEOF but do check for values in the fish reserved char range (e.g., R_EOF). Too, the signature of that function says it returns a wchar_t for which WEOF isn't valid (that symbol requires the function to return a wint_t).

What is really needed is a way for a core developer to reproduce this problem so we can validate my analysis.

@krader1961 krader1961 self-assigned this Jul 10, 2016
@krader1961
Member

@dewyatt, Can you build and install fish from "git head" source then do "exec fish" and still reproduce the problem? If yes I'd like to ask you to apply a patch to see if it fixes this problem for you.

@floam
Member
floam commented Jul 11, 2016 edited

This hopefully helps @krader1961, stepping through the whole thing on master.

https://gist.github.com/floam/519249caf016c0369f866ada4d5d879b#file-gistfile1-txt-L1098

@floam
Member
floam commented Jul 11, 2016

(It won't happen if I exec fish. I'm doing sudo su -l and closing Terminal.app, which tells me it will terminate su, sudo.)

@dewyatt
dewyatt commented Jul 11, 2016

@krader1961 I'm in job-search mode at the moment but will definitely come back to this in a week or so if floam's information doesn't help to resolve.

@krader1961
Member

The reason this problem isn't noticed if you exec fish after doing the su -l is because if the default shell isn't fish it has set the locale env vars. So score another reason for @faho's change to always attempt to set a sane, non-C, locale. If your default root shell is fish you can exec fish -d2 2>/tmp/fish.dbg and still reproduce the problem.

There are multiple bugs. The proximate one is that the C locale causes R_EOF to be truncated from 0xF701 to 0x01. So that explains the repeated "Unknown keybinding 1" diagnostics. That one is easy to fix.

There's probably another bug because doing exec su -l then closing the window doesn't trigger the problem. Similarly, simply forcing the C locale for the current, non-root user, fish shell then closing its window doesn't trigger the problem. This is probably related to some quirk in how process groups and the tty controlling terminal are managed by fish.

There's also the returning WEOF problem I mentioned in a previous comment. Changing that to return R_EOF doesn't fix the problem but also doesn't seem to break anything which suggests dead code or another weird, seldom triggered, corner case.

The debugging statements I added also show that 0x0D (\cD) is being returned by the low level input routines to signal EOF when the window is closed. That's clearly wrong as the special fish token R_EOF should be returned on EOF.

And then there's various bogosities like this statement

if ((b >= R_NULL) && (b < R_NULL + 1000)) return b;

in function input_common_readch(). The magic value "1000" is completely arbitrary and has nothing whatsoever to do with the number of special R_* symbols.

@krader1961
Member

@dewyatt, Just in case my previous comment didn't make it obvious I'm not going to need to impose on you any more to debug this issue.

@krader1961
Member
krader1961 commented Jul 12, 2016 edited

My previous comment about how \cD is handled is wrong. Fish deliberately disables the tty driver's handling of that character in order to handle it via a key binding. So it's reasonable for my debugging statements to report that readb() returns it. However, given there is a default binding to cause the shell to exit when that char is seen I'm perplexed why that's not happening.

@floam
Member
floam commented Jul 12, 2016 edited

Related?

SUMMARY: ThreadSanitizer: data race env.cpp:864 in env_push(bool)

WARNING: ThreadSanitizer: data race (pid=94374)
  Write of size 8 at 0x000106fb5358 by main thread:
    #0 env_push(bool) env.cpp:864 (fish+0x00010003c459)
    #1 parser_t::push_block(block_t*) parser.cpp:181 (fish+0x000100087482)
    #2 parser_t::eval_block_node(unsigned int, io_chain_t const&, block_type_t) parser.cpp:689 (fish+0x0001000898d3)
    #3 parser_t::eval_acquiring_tree(std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, io_chain_t const&, block_type_t, moved_ref<parse_node_tree_t>) parser.cpp:646 (fish+0x0001000896ac)
    #4 parser_t::eval(std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, io_chain_t const&, block_type_t) parser.cpp:621 (fish+0x000100088fe0)
    #5 exec_subshell_internal(std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, std::__1::vector<std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> >, std::__1::allocator<std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > > >*, bool) exec.cpp:1169 (fish+0x00010004a858)
    #6 exec_subshell(std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, std::__1::vector<std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> >, std::__1::allocator<std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > > >&, bool) exec.cpp:1217 (fish+0x00010004a6d6)
    #7 expand_cmdsubst(std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, std::__1::vector<completion_t, std::__1::allocator<completion_t> >*, std::__1::vector<parse_error_t, std::__1::allocator<parse_error_t> >*) expand.cpp:1084 (fish+0x00010004e482)
    #8 expand_stage_cmdsubst(std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, std::__1::vector<completion_t, std::__1::allocator<completion_t> >*, int, std::__1::vector<parse_error_t, std::__1::allocator<parse_error_t> >*) expand.cpp:1327 (fish+0x00010004ce87)
    #9 expand_string(std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, std::__1::vector<completion_t, std::__1::allocator<completion_t> >*, int, std::__1::vector<parse_error_t, std::__1::allocator<parse_error_t> >*) expand.cpp:1494 (fish+0x00010004cc25)
    #10 parse_execution_context_t::determine_arguments(parse_node_t const&, std::__1::vector<std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> >, std::__1::allocator<std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > > >*, parse_execution_context_t::globspec_t) parse_execution.cpp:929 (fish+0x0001000793ea)
    #11 parse_execution_context_t::populate_plain_process(job_t*, process_t*, parse_node_t const&) parse_execution.cpp:886 (fish+0x00010007aef0)
    #12 parse_execution_context_t::populate_job_process(job_t*, process_t*, parse_node_t const&) parse_execution.cpp:1120 (fish+0x00010007ba7a)
    #13 parse_execution_context_t::populate_job_from_job_node(job_t*, parse_node_t const&, block_t const*) parse_execution.cpp:1151 (fish+0x00010007bd62)
    #14 parse_execution_context_t::run_1_job(parse_node_t const&, block_t const*) parse_execution.cpp:1295 (fish+0x000100078a7e)
    #15 parse_execution_context_t::run_job_list(parse_node_t const&, block_t const*) parse_execution.cpp:1360 (fish+0x000100078e1d)
    #16 parse_execution_context_t::run_if_statement(parse_node_t const&) parse_execution.cpp:329 (fish+0x000100078596)
    #17 parse_execution_context_t::run_1_job(parse_node_t const&, block_t const*) parse_execution.cpp:1246 (fish+0x0001000788ad)
    #18 parse_execution_context_t::run_job_list(parse_node_t const&, block_t const*) parse_execution.cpp:1360 (fish+0x000100078e1d)
    #19 parse_execution_context_t::eval_node_at_offset(unsigned int, block_t const*, io_chain_t const&) parse_execution.cpp:1399 (fish+0x00010007c50a)
    #20 parser_t::eval_block_node(unsigned int, io_chain_t const&, block_type_t) parser.cpp:690 (fish+0x0001000898e5)
    #21 parser_t::eval_acquiring_tree(std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, io_chain_t const&, block_type_t, moved_ref<parse_node_tree_t>) parser.cpp:646 (fish+0x0001000896ac)
    #22 parser_t::eval(std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, io_chain_t const&, block_type_t) parser.cpp:621 (fish+0x000100088fe0)
    #23 internal_exec_helper(parser_t&, std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, unsigned int, block_type_t, io_chain_t const&) exec.cpp:336 (fish+0x00010004a1c3)
    #24 exec_job(parser_t&, job_t*) exec.cpp:675 (fish+0x000100048bc2)
    #25 parse_execution_context_t::run_1_job(parse_node_t const&, block_t const*) parse_execution.cpp:1326 (fish+0x000100078baf)
    #26 parse_execution_context_t::run_job_list(parse_node_t const&, block_t const*) parse_execution.cpp:1360 (fish+0x000100078e1d)
    #27 parse_execution_context_t::eval_node_at_offset(unsigned int, block_t const*, io_chain_t const&) parse_execution.cpp:1399 (fish+0x00010007c50a)
    #28 parser_t::eval_block_node(unsigned int, io_chain_t const&, block_type_t) parser.cpp:690 (fish+0x0001000898e5)
    #29 parser_t::eval_acquiring_tree(std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, io_chain_t const&, block_type_t, moved_ref<parse_node_tree_t>) parser.cpp:646 (fish+0x0001000896ac)
    #30 parser_t::eval(std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, io_chain_t const&, block_type_t) parser.cpp:621 (fish+0x000100088fe0)
    #31 input_mapping_execute(input_mapping_t const&, bool) input.cpp:466 (fish+0x000100068271)
    #32 input_mapping_execute_matching_or_generic(bool) input.cpp:533 (fish+0x00010006642c)
    #33 input_readch(bool) input.cpp:605 (fish+0x000100066053)
    #34 reader_readline(int) reader.cpp:2441 (fish+0x0001000967f1)
    #35 read_i() reader.cpp:2306 (fish+0x00010009c052)
    #36 reader_read(int, io_chain_t const&) reader.cpp:3409 (fish+0x00010009bd9e)
    #37 main fish.cpp:503 (fish+0x0001000ac89d)

  Previous read of size 8 at 0x000106fb5358 by thread T15 (mutexes: write M65):
    #0 env_get_string(std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, unsigned int) env.cpp:754 (fish+0x00010003b1b0)
    #1 expand_abbreviation(std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> >*) expand.cpp:1585 (fish+0x00010004dd57)
    #2 command_is_valid(std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, parse_statement_decoration_t, std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, env_vars_snapshot_t const&) highlight.cpp:1015 (fish+0x000100057ddf)
    #3 highlighter_t::highlight() highlight.cpp:1119 (fish+0x000100056eff)
    #4 highlight_shell(std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, std::__1::vector<unsigned int, std::__1::allocator<unsigned int> >&, unsigned long, std::__1::vector<std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> >, std::__1::allocator<std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > > >*, env_vars_snapshot_t const&) highlight.cpp:1195 (fish+0x000100057fcb)
    #5 background_highlight_context_t::perform_highlight() reader.cpp:2129 (fish+0x00010009cd09)
    #6 threaded_highlight(background_highlight_context_t*) reader.cpp:2169 (fish+0x00010009cfd9)
    #7 iothread_worker(void*) iothread.cpp:127 (fish+0x00010006f8f2)

  Location is global 'top' at 0x000106fb5358 (fish+0x0001000db358)

  Mutex M65 (0x000106fb49b0) created at:
    #0 pthread_mutex_lock <null>:149 (libclang_rt.tsan_osx_dynamic.dylib+0x0000000320be)
    #1 scoped_lock::lock() common.cpp:1785 (fish+0x00010002d7dc)
    #2 scoped_lock::scoped_lock(_opaque_pthread_mutex_t&) common.cpp:1796 (fish+0x00010002d975)
    #3 scoped_lock::scoped_lock(_opaque_pthread_mutex_t&) common.cpp:1796 (fish+0x00010002d9a0)
    #4 env_get_string(std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, unsigned int) env.cpp:752 (fish+0x00010003b1a4)
    #5 handle_curses(wchar_t const*) env.cpp:235 (fish+0x00010003eb6e)
    #6 react_to_variable_change(std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&) env.cpp:255 (fish+0x00010003c7b7)
    #7 env_set(std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, wchar_t const*, unsigned int) env.cpp:631 (fish+0x00010003ad74)
    #8 env_init(config_paths_t const*) env.cpp:383 (fish+0x00010003b714)
    #9 main fish.cpp:475 (fish+0x0001000ac462)

  Thread T15 (tid=772290, finished) created by main thread at:
    #0 pthread_create <null>:149 (libclang_rt.tsan_osx_dynamic.dylib+0x000000024350)
    #1 iothread_spawn() iothread.cpp:173 (fish+0x00010006eff2)
    #2 iothread_perform_base(int (*)(void*), void (*)(void*, int), void*) iothread.cpp:210 (fish+0x00010006eb3d)
    #3 int iothread_perform<background_highlight_context_t>(int (*)(background_highlight_context_t*), void (*)(background_highlight_context_t*, int), background_highlight_context_t*) iothread.h:33 (fish+0x00010005de29)
    #4 reader_super_highlight_me_plenty(int, bool) reader.cpp:2197 (fish+0x0001000998e3)
    #5 reader_set_buffer_maintaining_pager(std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, unsigned long) reader.cpp:1907 (fish+0x000100093607)
    #6 reader_set_buffer(std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, unsigned long) reader.cpp:1916 (fish+0x00010009536b)
    #7 builtin_commandline(parser_t&, io_streams_t&, wchar_t**) builtin_commandline.cpp:439 (fish+0x0001000149a7)
    #8 builtin_run(parser_t&, wchar_t const* const*, io_streams_t&) builtin.cpp:3178 (fish+0x000100006ac7)
    #9 exec_job(parser_t&, job_t*) exec.cpp:809 (fish+0x00010004918c)
    #10 parse_execution_context_t::run_1_job(parse_node_t const&, block_t const*) parse_execution.cpp:1326 (fish+0x000100078baf)
    #11 parse_execution_context_t::run_job_list(parse_node_t const&, block_t const*) parse_execution.cpp:1360 (fish+0x000100078e1d)
    #12 parse_execution_context_t::run_if_statement(parse_node_t const&) parse_execution.cpp:329 (fish+0x000100078596)
    #13 parse_execution_context_t::run_1_job(parse_node_t const&, block_t const*) parse_execution.cpp:1246 (fish+0x0001000788ad)
    #14 parse_execution_context_t::run_job_list(parse_node_t const&, block_t const*) parse_execution.cpp:1360 (fish+0x000100078e1d)
    #15 parse_execution_context_t::eval_node_at_offset(unsigned int, block_t const*, io_chain_t const&) parse_execution.cpp:1399 (fish+0x00010007c50a)
    #16 parser_t::eval_block_node(unsigned int, io_chain_t const&, block_type_t) parser.cpp:690 (fish+0x0001000898e5)
    #17 parser_t::eval_acquiring_tree(std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, io_chain_t const&, block_type_t, moved_ref<parse_node_tree_t>) parser.cpp:646 (fish+0x0001000896ac)
    #18 parser_t::eval(std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, io_chain_t const&, block_type_t) parser.cpp:621 (fish+0x000100088fe0)
    #19 internal_exec_helper(parser_t&, std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, unsigned int, block_type_t, io_chain_t const&) exec.cpp:336 (fish+0x00010004a1c3)
    #20 exec_job(parser_t&, job_t*) exec.cpp:675 (fish+0x000100048bc2)
    #21 parse_execution_context_t::run_1_job(parse_node_t const&, block_t const*) parse_execution.cpp:1326 (fish+0x000100078baf)
    #22 parse_execution_context_t::run_job_list(parse_node_t const&, block_t const*) parse_execution.cpp:1360 (fish+0x000100078e1d)
    #23 parse_execution_context_t::eval_node_at_offset(unsigned int, block_t const*, io_chain_t const&) parse_execution.cpp:1399 (fish+0x00010007c50a)
    #24 parser_t::eval_block_node(unsigned int, io_chain_t const&, block_type_t) parser.cpp:690 (fish+0x0001000898e5)
    #25 parser_t::eval_acquiring_tree(std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, io_chain_t const&, block_type_t, moved_ref<parse_node_tree_t>) parser.cpp:646 (fish+0x0001000896ac)
    #26 parser_t::eval(std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&, io_chain_t const&, block_type_t) parser.cpp:621 (fish+0x000100088fe0)
    #27 input_mapping_execute(input_mapping_t const&, bool) input.cpp:466 (fish+0x000100068271)
    #28 input_mapping_execute_matching_or_generic(bool) input.cpp:533 (fish+0x00010006642c)
    #29 input_readch(bool) input.cpp:605 (fish+0x000100066053)
    #30 reader_readline(int) reader.cpp:2441 (fish+0x0001000967f1)
    #31 read_i() reader.cpp:2306 (fish+0x00010009c052)
    #32 reader_read(int, io_chain_t const&) reader.cpp:3409 (fish+0x00010009bd9e)
    #33 main fish.cpp:503 (fish+0x0001000ac89d)
@floam
Member
floam commented Jul 12, 2016

Hm, no, think it is not.

@krader1961
Member

I think I've found another bug. In input.cpp, function input_mapping_is_match(), is this statement:

bool timed = (j > 0 && iswcntrl(str[0]));

AFAIK the only time we should apply a time limit to interactive input is if the first char of the sequence is the escape character — not an arbitrary control character. That's so we can disambiguate a bare escape from an escape that is a prefix to a longer sequence. This is primarily to facilitate vi-mode.

If we're going to allow a timeout for control chars that introduce other sequences why limit it to control chars? Why not every possible char? Yes, doing so would be somewhat silly but only because we allow binding arbitrary sequences; for example:

bind qed `echo WTF`

Go ahead. Start a shell and paste that statement then type "qed". Why shouldn't we timeout looking for an "e" after seeing a "q" just as we do when looking for another character after seeing an escape? Or other control char as currently implemented?

@krader1961
Member

With regard to my previous comment you might point out that timing out sequences that begin with a control char but are incomplete allows binding a subsequence that shares a prefix. For example:

bind \cYx 'echo WTF \\cYabc matched'
bind \cY 'echo WTF bare \\cY matched'

But, again, why is that any different from

bind qed 'echo WTF qed matched'
bind q 'echo WTF q matched'

It seems to me we should timeout when any prefix isn't followed in a timely manner by the rest of a sequence or we should limit this to just the escape character.

@ridiculousfish
Member

Looks like this was introduced in the rather uninformative commit a302f37 so I don't think this use of iswcntrl is justified. You described it as a bug - do you have a way of entering control characters other than escape?

@krader1961
Member

Argh! That commit comment is just "Fix bug". The bug appears to be in the key bindings so it's not clear why they changed the C++ code. Oh well.

I'm not going to change that logic as part of fixing this issue. But it does need to be changed as the current algorithm (as well as the behavior before that commit) is wrong. The correct algorithm is to detect whether the current input sequence is a prefix of a bind sequence. If it is do a timed wait for further input otherwise wait indefinitely for another character. That would correctly handle the "qed" example I gave above if the user types just "qe". The current algorithm waits indefinitely for the "d" or a char that doesn't complete the bind sequence because the first char isn't an escape or other control char. Sure, that's "good enough" 99.9% of the time but it's not correct.

There's something else weird going on that looks like a bug. Start a nested shell then enter

set fish_escape_delay_ms 500
bind \cYui 'echo WTF \\\\cYui matched'
bind \cY 'echo WTF bare \\\\cY matched'

Now press [ctrl-Y]ui and you should see the corresponding message. Type echo $SHLVL and you should see you're still in the nested shell. Now press just [ctrl-Y]. You should see its corresponding message but now if you echo $SHLVL you'll find the nested shell has terminated.

@krader1961 krader1961 added a commit to krader1961/fish-shell that referenced this issue Aug 10, 2016
@krader1961 krader1961 fix handling input in the C locale
In the C/POSIX locale EOF on the tty wasn't handled correctly due to a change
a few months ago to fix an unrelated problem with that locale. What is
surprising is that the core fish code doesn't explicitly depend on
input_common_readch returning WEOF if a character isn't seen within
`wait_on_escape_ms` after an escape.

Fixes #3214
3c06fc4
@krader1961 krader1961 referenced this issue Aug 10, 2016
Closed

fix handling input in the C locale #3296

2 of 2 tasks complete
@krader1961 krader1961 added a commit that closed this issue Aug 13, 2016
@krader1961 krader1961 fix handling input in the C locale
In the C/POSIX locale EOF on the tty wasn't handled correctly due to a change
a few months ago to fix an unrelated problem with that locale. What is
surprising is that the core fish code doesn't explicitly depend on
input_common_readch returning WEOF if a character isn't seen within
`wait_on_escape_ms` after an escape.

Fixes #3214
f3e93f0
@faho faho modified the milestone: next-2.x, fish-future Aug 21, 2016
@krader1961 krader1961 modified the milestone: fish 2.3.2, next-2.x Sep 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment