time,runtime: too many concurrent timer firings for short, fast-resetting time.Timer
#69969
Labels
compiler/runtime
Issues related to the Go compiler and/or runtime.
FixPending
Issues that have a fix which has not yet been reviewed or submitted.
NeedsFix
The path to resolution is known, but the work has not been done.
Milestone
Comments
gopherbot
added
the
compiler/runtime
|
Related Issues and Documentation
(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.) |
|
Change https://go.dev/cl/621616 mentions this issue: |
dmitshur
added
NeedsFix
|
@gopherbot Please open backport to 1.23 Parts of the change that this fixes have been backported to 1.23. We should complete the backport so that 1.23 includes all the fixes. |
|
Backport issue(s) opened: #69978 (for 1.23). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases. |
|
Change https://go.dev/cl/621856 mentions this issue: |
gopherbot
pushed a commit
that referenced
this issue
Oct 25, 2024
This change switches isSending to be an atomic.Int32 instead of an atomic.Uint8. The Int32 version is managed as a counter, which is something that we couldn't do with Uint8 without adding a new intrinsic which may not be available on all architectures. That is, instead of only being able to support 8 concurrent timer firings on the same timer because we only have 8 independent bits to set for each concurrent timer firing, we can now have 2^31-1 concurrent timer firings before running into any issues. Like the fact that each bit-set was matched with a clear, here we match increments with decrements to indicate that we're in the "sending on a channel" critical section in the timer code, so we can report the correct result back on Stop or Reset. We choose an Int32 instead of a Uint32 because it's easier to check for obviously bad values (negative values are always bad) and 2^31-1 concurrent timer firings should be enough for anyone. Previously, we avoided anything bigger than a Uint8 because we could pack it into some padding in the runtime.timer struct. But it turns out that the type that actually matters, runtime.timeTimer, is exactly 96 bytes in size. This means its in the next size class up in the 112 byte size class because of an allocation header. We thus have some free space to work with. This change increases the size of this struct from 96 bytes to 104 bytes. (I'm not sure if runtime.timer is often allocated directly, but if it is, we get lucky in the same way too. It's exactly 80 bytes in size, which means its in the 96-byte size class, leaving us with some space to work with.) Fixes #69978 For #69969. Related to #69880 and #69312 and #69882. Change-Id: I9fd59cb6a69365c62971d1f225490a65c58f3e77 Cq-Include-Trybots: luci.golang.try:go1.23-linux-amd64-longtest Reviewed-on: https://go-review.googlesource.com/c/go/+/621616 Reviewed-by: Ian Lance Taylor <iant@google.com> Auto-Submit: Michael Knyszek <mknyszek@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> (cherry picked from commit 6a49f81) Reviewed-on: https://go-review.googlesource.com/c/go/+/621856 Auto-Submit: Ian Lance Taylor <iant@google.com> Reviewed-by: Michael Pratt <mpratt@google.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This issue is the same as #69880 but for regular a
time.Timer. The difference between a regulartime.Timerand atime.Tickeris that it is much harder to reproduce the problem for atime.Timer. The timer must be getting reset at just the right time, with a very low duration, so that it fires again. And this needs to happen 8 times.The probability of this is very low, but it is possible to write code that causes this (though it's unclear if the code is useful). Slightly more concerning, however, is that there is an incredibly low chance of this just happening due to sheer luck.
I was basically only able to create a test for this that involves a timer that basically fires immediately (nanoseconds-long timer). Still, it is one additional possible problem.
Given that #69880 is fixed, fixing this isn't quite so important, but it turns out we can do so easily, to just close the door on this entirely.
It turns out that due to size class bumping from allocation headers, it costs us no extra memory to represent
isSendingas anatomic.Int32instead, so we can make the maximal number of concurrent timer firings something so absurdly large (2^31-1) that it will actually never happen.The text was updated successfully, but these errors were encountered: