search

manifoldco / promptui

Interactive prompt for command-line applications
https://www.manifold.co
BSD 3-Clause "New" or "Revised" License
6.07k stars 336 forks source link

Fix race conditions as mentioned in #148 #169

Open 1lann opened 3 years ago

1lann commented 3 years ago

This PR resolves the race conditions mentioned in #148.

The race condition was causing rendering glitches with prompts for me, it annoyed me sufficiently that I decided to investigate why the rendering was glitching and wrote a fix. Here's a PR for it if you'd like.

In summary what happens in most cases is when you hit "enter" in a prompt, an "ioloop" goroutine in the readline package dispatches the event to both the listen handler, and to the code that returns from .Readline() simultaneously. Because both the listen handler and the code that occurs after .Readline() returns in promptui mutate the readline screen buffer without synchronization, a data race occurs, and can cause rendering glitches depending on the order of execution (not to mention the data race itself is unsafe anyway).

This fix adds a mutex to synchronize the access between the listen handler and the after .Readline() return handling, additionally a closing variable is used to indicate to the listen handler when it should stop processing events as the prompt is quitting as to prevent the incorrect final text from being displayed.

I've tested this code on the example provided in #148, and with my own code, and it appears to be functioning great now without any rendering glitches in prompts, nor any warnings from Go's race detector when running with -race.

Reminder that after merging and testing, you'll probably want to release this as an update.

CLAassistant commented 3 years ago

CLA assistant check
All committers have signed the CLA.

aschrijver commented 3 years ago

Liking the project, but would be great to see this merged. I just upgraded from v0.3.2 to do the bell fix, but now with duplicate prompt lines and inconsistent formatting (prompt bold, not bold, at random) that results from this it feels a bit like a step backwards.

viccuad commented 3 years ago

I would be glad to help in merging this PR, as I depend on it to use the module.

kylecarbs commented 2 years ago

What's left to get this merged?

johnmanjiro13 commented 2 years ago

I want you to merge this PR.

kumailn commented 2 years ago

@1lann when will this be merged?

1lann commented 2 years ago

@kumailn I do not have push access to this repository, if I could merge it I would, the best I can do is resolve the merge conflict (which I have now done). You'd have to go poke the maintainers of this repository if you want to get this merged. Alternatively you can use my fork at https://github.com/1lann/promptui, I haven't tested it however so use at your own risk. Alternatively you can use a library that is better maintained like bubletea which is what I've ended up doing. A text input example is here, you might want to write a helper to make it more elegant to use.

AaronCrawfis commented 2 years ago

@jbowes @louisbranch would you be able to take a look at this? Would love to fix an issue we have for double prompts on Windows. Thanks!!

ish-xyz commented 2 years ago

@jbowes @louisbranch can you please have a look here?

maidul98 commented 3 months ago

Just ran into the same issue. Is there a blocker to mergeing this?