fix(swarm): prevent overflow in keep-alive computation

[ghost]

Description

Add safe check to prevent Delay::reset() panic when duration overflow.

Fixes: #4641.

Attributions

Co-authored-by: Leonz [email protected]
Co-authored-by: Max Inden [email protected]

Notes & open questions

Change checklist

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• A changelog entry has been made in the appropriate crates

[mxinden]

Thank you for the fast turnaround time. Overall looks good to me.

@thomaseizinger mind taking a look given that this is quite subtle?

@zrus can you add an entry to swarm/CHANGELOG.md?

[mxinden]

Thank you for the follow-up. Again, let's give Thomas a chance to review. The more eyes on this subtle code path, the better.

[thomaseizinger]

Thanks! Embarrassing that we didn't catch this in the first patch.

I'd like to test this better. In #4595, we are extracting this big code block into a function. Would you be up for copying that to this PR and writing some prop-tests using quickcheck for it?

[ghost]

I'd like to test this better. In #4595, we are extracting this big code block into a function. Would you be up for copying that to this PR and writing some prop-tests using quickcheck for it?

I have done it. But I have not added any tests yet. Please help me on this.

[thomaseizinger]

Thanks!

Regarding tests, are you familiar with quickcheck? https://github.com/BurntSushi/quickcheck

[ghost]

Regarding tests, are you familiar with quickcheck? https://github.com/BurntSushi/quickcheck

I have not used it before. Could you guide me?

[thomaseizinger]

Regarding tests, are you familiar with quickcheck? BurntSushi/quickcheck

I have not used it before. Could you guide me?

Sure! You'll like this :)

The idea of quickcheck is to property-based testing by generate test-cases automatically asserting no the code having certain properties. We can use that for this code to check that regardless of which inputs we generate, the code never pancis but always yields a valid Shutdown value.

You can find an example test in the connection.rs file: max_negotiating_inbound_streams.

To write your own test, you need to create a prop function (name doesn't matter) that takes the inputs you want quickcheck to permute. The function needs to call compute_new_shutdown. To start with, we don't need to make any further assertions, simply not panicking is good already!

We'll need a couple of things:

1. Add an implementation of quickcheck::Arbitrary to KeepAlive. The implementation should be gated by #[cfg(test)].
2. Generalize the KeepAliveUntilConnectionHandler to take an entire KeepAlive and not just an Instant.
3. Write a quickcheck-test with a prop function that takes a KeepAlive and a Duration:
   • The test should make a new KeepAliveUntilConnectionHandler with the passed in KeepAlive
   • Call compute_new_shutdown with Shutdown::None, the handler and the provided Duration as idle_timeout

That should get us started. We can then think about whether we want to permute the existing Shutdown as well which is a bit trickier.

[thomaseizinger]

Tagging @leonzchang for visibility of related effort. We cherry-picked some of your code to make testing this easier :)

I added you as a co-author to the PR description to correctly attribute your contribution!

[mergify]

This pull request has merge conflicts. Could you please resolve them @zrus? 🙏

[mxinden]

In order to unblock #4625, I am looking into the quickcheck test right now.

[mxinden]

Thank you for your work!

[thomaseizinger]

Thanks @mxinden !

Added some ideas that we can tackle in a follow-up :)

[thomaseizinger]

@mxinden There are merge conflicts unfortunately.

Approvals have been dismissed because the PR was updated after the send-it label was applied.

[mxinden]

@thomaseizinger assuming that with all comments addressed, you are fine with me merging here.

Approvals have been dismissed because the PR was updated after the send-it label was applied.

[ghost]

I apologize for disappearing suddenly from last conversation about the test. I was too busy last week. And thank both of you, @thomaseizinger, @mxinden, for the great work and being patient with me.

[thomaseizinger]

I apologize for disappearing suddenly from last conversation about the test. I was too busy last week. And thank both of you, @thomaseizinger, @mxinden, for the great work and being patient with me.

No worries at all! Normally nothing is super urgent but we wanted to start the next breaking release and this felt important enough to be patched before that :)