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

Fix race conditions as mentioned in #148 #169

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

1lann
Copy link

@1lann 1lann commented Dec 31, 2020

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
Copy link

CLAassistant commented Dec 31, 2020

CLA assistant check
All committers have signed the CLA.

@1lann 1lann changed the title Fixes race conditions as mentioned in #148 Fixe race conditions as mentioned in #148 Dec 31, 2020
@1lann 1lann changed the title Fixe race conditions as mentioned in #148 Fix race conditions as mentioned in #148 Dec 31, 2020
@1lann 1lann mentioned this pull request Dec 31, 2020
@aschrijver
Copy link

aschrijver commented Jan 12, 2021

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
Copy link

viccuad commented May 10, 2021

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

@kylecarbs
Copy link

What's left to get this merged?

@johnmanjiro13
Copy link

I want you to merge this PR.

@kumailn
Copy link

kumailn commented Jul 8, 2022

@1lann when will this be merged?

@1lann 1lann force-pushed the 1lann/fix-race-condition branch from d8f2159 to 8bcc42a Compare July 8, 2022 22:27
@1lann
Copy link
Author

1lann commented Jul 8, 2022

@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
Copy link

@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
Copy link

ish-xyz commented Nov 4, 2022

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

@maidul98
Copy link

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

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

Successfully merging this pull request may close these issues.

10 participants