retry 3 times before displaying an error

[MaxVerevkin]

Closes #1891 Closes #1838

[bim9262]

So this will change the behaviour from trying again every 5 seconds (and showing an error) to trying three times with 1 and 2 second delays between attempts before displaying an error and then sleeping for 30 seconds? That 30 second window seems like it's potentially a really long timeout depending on what failed.

[MaxVerevkin]

So this will change the behaviour from trying again every 5 seconds (and showing an error) to trying three times with 1 and 2 second delays between attempts before displaying an error and then sleeping for 30 seconds?

Yes.

That 30 second window seems like it's potentially a really long timeout depending on what failed.

True. It can be reverted back to five.

[MaxVerevkin]

By the way, I noticed README states that

When the state is "Error", a short error will be displayed in the block. The full message can be toggled by clicking on the block (overrides any click actions defined in the config). The block will be restarted after error_interval has elapsed.

Which is not what is happening right now. But maybe we can literally restart the failed block and get rid of all the .recoverable() calls? Then only the blocks that need retry policy (basically those that make web requests) will have it. This wasn't possible before async rewrite because some blocks used to spawn threads, but it is not as issue any more. Any thoughts?

[bim9262]

I like the sound of that 👍

[MaxVerevkin]

One annoying thing is that blocks::CommonApi is not Clone because of event_receiver, which is needed for the custom_dbus block, which doesn't even use event_receiver.

[MaxVerevkin]

But maybe we can literally restart the failed block and get rid of all the .recoverable() calls?

This would change the behavior of some blocks like weather. For example, if autolocate_interval is "once", I unfortunately don't see way to restart the block without autolocating again.

[ammgws]

We could perhaps store state for those on disk if it must absolutely respect "once"

[MaxVerevkin]

Never mind, the last autolocate result can be stored in a static mutex.

[MaxVerevkin]

Implementing #1838 turned out to be easier than I thought. However I'm not convinced that https://github.com/greshake/i3status-rust/pull/1912/commits/453bc9de3f7db2c4181d0b4a561ca1922f3a7f73 doesn't break anything, testing is needed. But I haven't noticed any issues.

[bim9262]

So far my testing hasn't shown any issues caused by https://github.com/greshake/i3status-rust/commit/453bc9de3f7db2c4181d0b4a561ca1922f3a7f73.

Do you think that using action.as_ref() instead of &*action is easier to read? (or is there some other downside to that's not not obvious...)

-                Some(action) = actions.recv() => match &*action {
+                Some(action) = actions.recv() => match action.as_ref() {

[MaxVerevkin]

Do you think that using action.as_ref() instead of &*action is easier to read?

I personally don't see much difference, but if you do, we can change it.

or is there some other downside to that's not not obvious...

I guess it is just a matter of preference.

[bim9262]

I don't mind either way, but I think that maybe as_ref will be a bit more intuitive to newcomers who wish to contribute.

[bim9262]

lgtm @MaxVerevkin