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

readline support #28

Closed
slingamn opened this issue Feb 6, 2023 · 6 comments
Closed

readline support #28

slingamn opened this issue Feb 6, 2023 · 6 comments

Comments

@slingamn
Copy link
Member

slingamn commented Feb 6, 2023

History: #12, #19

Status:

  1. We are affected by a bug in the readline library: (*Instance).Close() hangs if there is a concurrent (*Instance).ReadLine call chzyer/readline#217
  2. The readline library uses obsolete hacks on Windows for ANSI replacement
  3. The readline library does not seem very trustworthy/reliable in general
@slingamn
Copy link
Member Author

slingamn commented Feb 7, 2023

Testing on Windows: the legacy ANSI support in chzyer/readline v1.5.1 actually produces a crash:

goroutine 14 [running]:
github.com/chzyer/readline.(*ANSIWriterCtx).ioloopEscSeq(0xc0000243c0?, 0xfe6142?, 0x6d, 0xc00001ef98)
        /redacted/ircdog/vendor/github.com/chzyer/readline/ansi_windows.go:164 +0x66f
github.com/chzyer/readline.(*ANSIWriterCtx).process(0xc00001ef90, 0x1b?)
        /redacted/ircdog/vendor/github.com/chzyer/readline/ansi_windows.go:99 +0xcf
github.com/chzyer/readline.(*ANSIWriter).Write(0xc00001efc0, {0xc000018640, 0xad, 0x140})
        /redacted/ircdog/vendor/github.com/chzyer/readline/ansi_windows.go:207 +0xdf
github.com/chzyer/readline.(*wrapWriter).Write.func1()
        /redacted/ircdog/vendor/github.com/chzyer/readline/operation.go:57 +0x44
github.com/chzyer/readline.(*RuneBuffer).Refresh(0xc000155cc0, 0xc00017bd40)
        /redacted/ircdog/vendor/github.com/chzyer/readline/runebuf.go:463 +0xd8
github.com/chzyer/readline.(*wrapWriter).Write(0xc00017bdb0, {0xc000018640?, 0xded668?, 0xc000021110?})
        /redacted/ircdog/vendor/github.com/chzyer/readline/operation.go:56 +0xa7
github.com/chzyer/readline.(*Instance).Write(0xc000021110?, {0xc000018640, 0xad, 0x140})
        /redacted/ircdog/vendor/github.com/chzyer/readline/readline.go:298 +0x85
fmt.Fprintln({0x1114660, 0xc000008378}, {0xc00017bef8, 0x1, 0x1})
        /usr/local/go/src/fmt/print.go:305 +0x75
main.connectExternal.func2()
        /redacted/ircdog/ircdog.go:303 +0x305
created by main.connectExternal
        /redacted/ircdog/ircdog.go:284 +0x2d9

In my fork that replaces it with native Windows support using the technique from https://github.com/jwalton/go-supportscolor , there is no crash, but basic readline functionality (e.g. the up arrow) is broken.

@slingamn
Copy link
Member Author

slingamn commented Feb 7, 2023

Leaning towards: readline support can be compiled into the binary by default, but pending additional work on a fork of the library, it will require an explicit command-line argument to enable.

@slingamn
Copy link
Member Author

slingamn commented Feb 7, 2023

Even without using the readline library, the up arrow works with ircdog in Command Prompt and PowerShell?!

It works even in this simple test case, although I can't find it documented anywhere: https://gist.github.com/slingamn/f19e8b7d8b1098d177fea15779fb9281

@slingamn slingamn mentioned this issue Feb 7, 2023
@slingamn
Copy link
Member Author

slingamn commented Feb 7, 2023

The library also has data races: chzyer/readline#214

that are nontrivial to remove. Every component talks to every component, so locking at the component level risks deadlock. Locking at the top level risks stalling the application (if the lock ends up being held during a blocking write operation of some sort).

@slingamn
Copy link
Member Author

Status:

  1. We have a patchset that enables experimental (opt-in) use of readline: https://github.com/slingamn/readline/commits/experimental
  2. Opt-in is via command-line argument --readline or environment variable IRCDOG_READLINE=1
  3. We have an open dialogue on Various screen redraw fixes for wide characters, narrow screens etc. chzyer/readline#202 with other users / potential maintainers of the library
  4. Readline is pretty compelling, I'm going to leave this open until we are ready to switch to opt-out readline. (--raw will always opt out; we should also support --no-readline and IRCDOG_READLINE=0.)

@slingamn
Copy link
Member Author

slingamn commented Jun 2, 2023

Actually resolved in #41!

@slingamn slingamn closed this as completed Jun 2, 2023
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

No branches or pull requests

1 participant