Skip to content

fix: TimeoutTicker returns wrong value/timeout pair when timeouts are scheduled at ~approximately the same time (backport #3092)#3105

Merged
sergio-mena merged 1 commit intov1.xfrom
mergify/bp/v1.x/pr-3092
May 22, 2024
Merged

fix: TimeoutTicker returns wrong value/timeout pair when timeouts are scheduled at ~approximately the same time (backport #3092)#3105
sergio-mena merged 1 commit intov1.xfrom
mergify/bp/v1.x/pr-3092

Conversation

@mergify
Copy link
Contributor

@mergify mergify bot commented May 22, 2024

#3091

The problem is we have an edge case where we should drain the timer channel, but we "let it slide" in certain race conditions when two timeouts are scheduled near each other. This means we can have unsafe timeout behavior as demonstrated in the github issue, and likely more spots in consensus.

Notice that aside from NewTimer and OnStop, all timer accesses are from the same thread. In NewTimer we can block until the timer is drained (very quickly up to goroutine scheduling). In OnStop we don't need to guarantee draining before the method ends, we can just launch something into the channel that will kill it.

In the main timer goroutine, we can safely maintain this "timerActive" variable, and force drain when its active. This removes the edge case.

The test I created does fail on main.


PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments
  • Title follows the Conventional Commits spec

This is an automatic backport of pull request #3092 done by [Mergify](https://mergify.com).

… scheduled at ~approximately the same time (#3092)

#3091

The problem is we have an edge case where we should drain the timer
channel, but we "let it slide" in certain race conditions when two
timeouts are scheduled near each other. This means we can have unsafe
timeout behavior as demonstrated in the github issue, and likely more
spots in consensus.

Notice that aside from NewTimer and OnStop, all timer accesses are from
the same thread. In NewTimer we can block until the timer is drained
(very quickly up to goroutine scheduling). In OnStop we don't need to
guarantee draining before the method ends, we can just launch something
into the channel that will kill it.

In the main timer goroutine, we can safely maintain this "timerActive"
variable, and force drain when its active. This removes the edge case.

The test I created does fail on main.

---

#### PR checklist

- [X] Tests written/updated
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [X] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

(cherry picked from commit 153281a)
@mergify mergify bot requested a review from a team as a code owner May 22, 2024 13:35
@mergify mergify bot requested a review from a team May 22, 2024 13:35
@sergio-mena sergio-mena self-assigned this May 22, 2024
@sergio-mena sergio-mena added the bug Something isn't working label May 22, 2024
@sergio-mena sergio-mena merged commit 7d3cf13 into v1.x May 22, 2024
@sergio-mena sergio-mena deleted the mergify/bp/v1.x/pr-3092 branch May 22, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants