rework Ticker (fix #416)

[chris-laplante]

Here's a first draft of how I think Ticker should work.

So far the only testing I've done is to confirm that the long-spinner example works OK. I have not yet tested the modified version of it (which uses the Barriers).

Fixes #416.

TODO:

• Write tests

[djc]

If you're moving code around that should be in a separate commit from any other changes you make, otherwise reviewing this becomes very hard. I'm not convinced that the Ticker is large enough to warrant a separate module, so I think we should probably hold off on that refactoring until the end. If you start with adding the test I will probably try to relentlessly simplify while still passing the test, as I think this looks like way too much stuff...

[chris-laplante]

If you're moving code around that should be in a separate commit from any other changes you make, otherwise reviewing this becomes very hard. I'm not convinced that the Ticker is large enough to warrant a separate module, so I think we should probably hold off on that refactoring until the end. If you start with adding the test I will probably try to relentlessly simplify while still passing the test, as I think this looks like way too much stuff...

Understood - I've moved the code back. I'd be curious what you think looks superfluous? Maybe TickerControl could be replaced with a tuple, though I don't think that would help readability.

[chris-laplante]

Also just to add some context for the changes - instead of adding a ProgressBarInner, I realized it was easier to just pull the Ticker up a level from BarState into ProgressBar.

[djc]

I feel like this is over-indexing on "we must join the thread" instead of solving the original problem of "we should make sure to do run a finish draw even if the ticker happens to hold the lock while the parent progress bar is finishing", resulting in a lot of stuff -- doubling the amount of code relating to the ticker and pulling in the additional CondVar type. I think the other idea of having a Drop impl block on the Ticker being done would be simpler and just as effective?

[chris-laplante]

I feel like this is over-indexing on "we must join the thread" instead of solving the original problem of "we should make sure to do run a finish draw even if the ticker happens to hold the lock while the parent progress bar is finishing", resulting in a lot of stuff -- doubling the amount of code relating to the ticker and pulling in the additional CondVar type. I think the other idea of having a Drop impl block on the Ticker being done would be simpler and just as effective?

IMHO a condition variable is the classic way to approach a problem like this (at least in C++), but if you can think of a different way I'd be happy to consider.

[djc]

Yeah, so the way I have in mind is that we move the Drop impl from BarState to another newtype that wraps the Mutex<BarState>, and finishes the bar only once it can lock the Mutex.

[chris-laplante]

Yeah, so the way I have in mind is that we move the Drop impl from BarState to another newtype that wraps the Mutex<BarState>, and finishes the bar only once it can lock the Mutex.

Sure, as long as we can guarantee that the drop doesn't happen inside the Ticker thread, otherwise (unless we join the thread somewhere) the drop might not happen.

[chris-laplante]

I can't think of a way to write a test for this without ensuring the thread is joined.

@djc were you going to try to implement your proposal, or did you want me to take a stab at it?

[djc]

I was hoping you'd write a test, once that exists I'm happy to try my hand at a fix. Does that work for you?

[djc]

As for the test, can we include the barriers in a test somehow? Or build a render test out of the long spinner example?

[chris-laplante]

As for the test, can we include the barriers in a test somehow? Or build a render test out of the long spinner example?

Sure, I'll give it a shot!

[chris-laplante]

I don't think there is a good way to write a test for this :/. The Barrier approach is helpful to force the race to manifest, but I don't think there is any way to use them to assert that a race doesn't exist. At the very least, adding Barriers only in #cfg[test] will add synchronization that then isn't present in the release build.

I think the best way forward would be for you to give your approach a shot. The "test" may be for me to try to insert Barriers into it to break it like I did with the current Ticker code.

[chris-laplante]

Actually, I may have just thought of a way... Will open a new PR with the test if I can get it to work.

[chris-laplante]

It's been a while but I'm back at this... I can't figure out a way to test this particular implementation without using a nightly feature (JoinHandle::is_finished), but I am still pretty certain that this is the correct approach.

I thought of a way to test it.

[djc]

I've been working on a bunch of other stuff (notably bringing chrono back to life), but am still thinking about this. I still feel like the requirement to join threads is too strong. As I understand it, the goal is that a non-ticker thread has the final draw, and it feels like we should be able to achieve that without having to join the thread. In fact, I think another goal here is that the non-ticker thread's drop of the ProgressBar should not wait for the ticker thread to be done with sleeping, since the sleep interval can be arbitrarily long.

[chris-laplante]

In fact, I think another goal here is that the non-ticker thread's drop of the ProgressBar should not wait for the ticker thread to be done with sleeping, since the sleep interval can be arbitrarily long.

FWIW this CondVar based approach does accomplish this, since the wait will be interrupted :)

[djc]

I pushed a work-in-progress approach that includes the barrier-based test to the ticker-termination-test, it involves adding another Mutex. I think that could work, but it's not quite there yet -- the issue is in figuring out exactly how the progress bar state mutex and the ticker mutex should overlap.

Can you articulate what a CondVar solves here that a Mutex cannot?

[chris-laplante]

Can you articulate what a CondVar solves here that a Mutex cannot?

The CondVar lets us say "wait until timeout has elapsed OR we are signaled to stop". Condition variable is its own unique synchronization primitive so it's hard to compare to a plain mutex.

[chris-laplante]

Also, I'm not convinced that joining the thread is too strong of a guarantee though I concede it may be. But at the end of the day, I'd argue it is much easier to reason about than if you try to build something with a bunch of mutexes. And it leads to simpler code.

[djc]

(Please ping me explicitly when you think this is ready for review -- or switch it back to draft status while it's not.)

[chris-laplante]

@djc sorry, yes I think this is ready. I've been testing it locally and no problems so far (in fact I was running this code when I discovered #424)

[chris-laplante]

@djc - any further thoughts on this? I'd love to get it merged in soon. It has been working for me so far in all my testing.

[djc]

Here's an initial round of feedback. I expect I'll have some more things after this stuff has been addressed.

[chris-laplante]

Suggestions applied. Sorry, I didn't do it by squashing since it was too annoying (because the structs moved).

[djc]

Bunch of nits, but I think this is pretty close!

[djc]

Nice, thanks for seeing this through!

[chris-laplante]

Nice, thanks for seeing this through!

Of course, thanks for your guidance and feedback :)

[stuhood]

Hey folks!

A bisect between 0.16.2 and 0.17.0 showed that this PR broke spinner updates for our use case. The 0.16.2->0.17.0 update diff looks like this: pantsbuild/pants@main...stuhood:stuhood/indicatif-0.17

I don't see anything in the 0.17.0 release notes about changes to expectations around ticking: should we be manually calling tick, or is it required to enable_steady_tick now? One slightly unusual thing about our usecase is that we only use bar.set_message, and no other progress reporting methods.

[djc]

@stuhood sorry about that, and thanks for doing the bisection! Can you file a new issue to track this problem with a minimal reproduction of your issue?

[chris-laplante]

Sorry about the breakage :(

One slightly unusual thing about our usecase is that we only use bar.set_message, and no other progress reporting methods.

Yes, I think that's what's doing it. A short-term fix should be to add a call to tick after you set_message.