Address issue 2473

[fegies]

Motivation

Resetting expired timers can cause panics (see #2473)

Solution

Add an additional check in the remove function fixes this issue.

Please feel free to discard the second commit if you think it is incorrect / correct me if I put the test in the wrong location.

[Darksonn]

Can you please merge master? That should fix CI.

[fegies]

Rebased onto master

[Darksonn]

This seems wrong. The DelayQueue should not try to remove things from the wheel that are not in the wheel. It seems like a special case in reset_at would be more appropriate, similar to what DelayQueue::remove does:

tokio/tokio/src/time/delay_queue.rs

Lines 461 to 466 in d2f81b5

	// Special case the `expired` queue
	if self.slab[key.index].expired {
	self.expired.remove(&key.index, &mut self.slab);
	} else {
	self.wheel.remove(&key.index, &mut self.slab);
	}

That said, I'm not sure and spent a long time staring at this trying to figure out what the variables really mean.

[jxs]

The DelayQueue should not try to remove things from the wheel that are not in the wheel.

ah sorry, I thought it was supposed to as the doc states the function panics if key is not contained by the queue.

[Darksonn]

Well, the DelayQueue distributes the timeouts into two collections, namely wheel and expired, so just because the key is in the DelayQueue, that does not mean that it is also in the wheel.

[fegies]

Well, the DelayQueue distributes the timeouts into two collections, namely wheel and expired, so just because the key is in the DelayQueue, that does not mean that it is also in the wheel.

Which is the source of the problem causing the panic.

I can move the test to reset_at if you think this is more appropriate (should work either way)

[Darksonn]

Yeah, I think it is better to have DelayQueue use the api as exposed by the wheel, and have the check in reset_at instead.

[fegies]

Replaced the fix commit with a check in the DelayQueue