-
Notifications
You must be signed in to change notification settings - Fork 123
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
bar.Abort should not cause the bar to render if the render delay hasn't expired #136
Comments
enable renderReq only after delayRC has been fired
Should be fixed. If you can test against latest master branch, please do. I didn't test your example thoroughly. |
@vbauerster Thanks for the quick response. Alas, this does not fix the issue. I've distilled my example down to the bare minimum. // require github.com/vbauerster/mpb/v8 v8.7.1-0.20231205062852-da3162c67234
func main() {
const delay = time.Second * 5
start := time.Now()
delayCh := make(chan struct{})
time.AfterFunc(delay, func() { close(delayCh) })
p := mpb.New(mpb.WithAutoRefresh(), mpb.WithRenderDelay(delayCh))
bar := p.New(0, mpb.BarStyle(), mpb.BarRemoveOnComplete())
bar.Abort(true)
bar.Wait() // <-- blocks here until render delay completes
p.Wait()
fmt.Println("Elapsed: ", time.Since(start))
} Running this example, you'll see that it blocks on $ go install github.com/neilotoole/mpb-render-delay@latest
$ mpb-render-delay
Elapsed: 5.002145041s BTW, note that that the behavior is the same with or without |
I don't see any wrong behavior here. Render delay is respected correctly, bar is waiting for a reason. If you don't want to wait for a bar don't call |
@vbauerster: Even if I skip the func main() {
const delay = time.Second * 5
start := time.Now()
delayCh := make(chan struct{})
time.AfterFunc(delay, func() { close(delayCh) })
p := mpb.New(mpb.WithAutoRefresh(), mpb.WithRenderDelay(delayCh))
bar := p.New(0, mpb.BarStyle(), mpb.BarRemoveOnComplete())
bar.Abort(true)
// bar.Wait()
p.Wait() // <-- blocks here until render delay completes
fmt.Println("Elapsed: ", time.Since(start))
} Why should If we run the example without creating any bars: func main() {
const delay = time.Second * 5
start := time.Now()
delayCh := make(chan struct{})
time.AfterFunc(delay, func() { close(delayCh) })
p := mpb.New(mpb.WithAutoRefresh(), mpb.WithRenderDelay(delayCh))
p.Wait() // <-- Doesn't block, because no "alive" bars.
fmt.Println("Elapsed: ", time.Since(start))
} Then this will finish without waiting for the render delay, as expected 👍. The behavior should be the same if all of That is to say, it should be functionally equivalent to invoke |
|
@vbauerster Yes, I looked at the source code, and I understand your reluctance to address the issue. I think it would be a challenging fix. I will document my understanding of the issue below, in case any other users encounter it. Problem summary
In the scenario I'm working with, some bars will complete before the render delay, and some will complete after the render delay. In both cases, the bar is to be removed. The expectation is that the short-lived bars are never rendered, because they were born and died before the render delay expired. This is not what happens. ExampleTake this example: $ go install github.com/neilotoole/mpb-render-delay@example-six-seconds
$ mpb-render-delay This causes a single-frame "flash" of the aborted (already-dead) bars at the point of the render-delay expiration, and then proceeds correctly. I've pulled this out into a super-slowmo video (in reality, it's just a single frame): mpb-render-delay-slowmo.mp4Obviously this isn't the desired outcome. Next stepsPer @vbauerster's comment above, the interaction between However, the exhibited "flash" behavior isn't acceptable in my use case. I suspect I will address the issue by implementing the render delay in an outer layer of decorators (for It's ugly, but it'll probably work. If I do manage to make it work, I'll follow up with a link to it here, for any other users who encounter the issue. @vbauerster, thank you for taking the time to look at this issue. If you do ever create a v9 or revisit the rendering pipeline, maybe this scenario can be handled? Thanks again for a great library. |
instead blocking renderReq, discard any output until delayRC fires.
Thanks for detailed example, it helped me to apply I believe correct fix, at least "flash" behavior is gone when I tested it. |
@vbauerster I can confirm that the "flash" seen in the example is fixed on Keep up the great work, and thanks for being so responsive. It's a wonderful project. |
@vbauerster In my (limited) testing, I haven't found any issues with the fix. Closing 👍 |
First, thanks for a wonderful library 🙏
Problem
I create a progress container with a render delay of, say, 5s. I have multiple bars, some of which complete quickly, and some which take a long time. When a bar completes its work, I call
bar.Abort(true)
. However, if Abort is called before the render delay has expired (e.g. in 2s), the bar will, surprisingly, render immediately.What I expect to happen
I expect
Abort
to honor the render delay. If a bar completes its work before the render delay expires, the bar should never be rendered.See full example in gist: https://gist.github.com/neilotoole/0c799d06ef9d47458328bdfd6054790a
The text was updated successfully, but these errors were encountered: