这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@henvic
Copy link
Contributor

@henvic henvic commented May 14, 2016

This should close #18 and #20.

@mgurov, can you take a look at the changes I have made on top of your code (which I actually rebased), please?

gosuri/uilive#7 is also needed.

… flush happens after the listener is stopped.
progress.go Outdated
default:
time.Sleep(p.RefreshInterval)

go func() {
Copy link
Contributor

@mgurov mgurov May 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exported function .Listen() changes its behavior from doing the listen loop to spawning a goroutine and exiting without waiting for the end of the processing. Which is a breaking change, IMHO and should probably be avoided.

@mgurov
Copy link
Contributor

mgurov commented May 15, 2016

@henvic works for me from the functional point of view.

@henvic
Copy link
Contributor Author

henvic commented May 19, 2016

@mgurov, what about now? I have done the same changes on uilive as well.

@mgurov
Copy link
Contributor

mgurov commented May 20, 2016

@henvic LGTM now. My comments are addressed. Thank you!

@henvic
Copy link
Contributor Author

henvic commented May 20, 2016

Hey @mgurov,

If you run the tests repeatedly (just do while true; do go test; done) you will see they fail from time to time (even though my examples are running flawlessly).

I have tried adding even a time.Sleep(2 * p.RefreshInterval) on the Stop() of uiprogress and uilive just after close(), but it doesn't do any difference. Any ideas of what may be going on? Thanks.

@mgurov
Copy link
Contributor

mgurov commented May 22, 2016

@henvic two things.

First, if you run make test repeatedly you'll soon hit race conditions because the newly added test uses unsynchronized bytes.Buffer for progress and for fmt.Fprintf(buffer, "foo"). Adding some form of synchronization would help.

Second, as far as I can see there is no strict guarantee that your fmt.Fprintf(buffer, "foo") will happen after the last flush at the case <-p.ticker.C of the Listener(). Say the signal from the ticker arrived at the same moment Stop() has just acquired the write lock. Ticker handling will proceed after the Stop() is done, on a collision course with the fmt.Fprintf, occasionally leading to a suffix different than you expect in the test.

@henvic
Copy link
Contributor Author

henvic commented May 24, 2016

@mgurov,
why would you hit race conditions given that while true; do go test; done runs several, independent processes, and in series (as in, each one after the previous exited)? It doesn't make sense for me.

Actually note that this might happen even for just one test. I said to run it repeatedly because it'd be easier for the issue to be replicated.

@henvic
Copy link
Contributor Author

henvic commented May 24, 2016

I think with these last changes the synchronization issue is gone.

@henvic henvic force-pushed the fix-race-condition-on-stop branch from 91304f9 to e3db2fd Compare May 26, 2016 02:23
@xorpaul
Copy link

xorpaul commented Oct 14, 2016

👍 for merge

@henvic
Copy link
Contributor Author

henvic commented Oct 14, 2016

@xorpaul, if you are blocked while these fixes doesn't make mainstream you can temporarily use my fork (or better, do a fork of this repo yourself and do what I did in order for you to keep track you're not using the official version). It is set with the fix-race-condition-on-stop-henvic branch as the default one and the uilive dependency there is pointing to a uilive with these fixes as well, so you can just 'go get' and use it (or do the same for uilive). Just need to replace gosuri with henvic on your code.

@xorpaul
Copy link

xorpaul commented Oct 14, 2016

@henvic Yeah, thanks for the suggestion, but I am already using your fork 😄

@gosuri gosuri merged commit 1de4226 into gosuri:master Feb 24, 2017
@gosuri
Copy link
Owner

gosuri commented Feb 24, 2017

great! thanks so much

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.

Need a way to wait progress bar output its final state

4 participants