First, skip all the allocation count tests.
In some cases this aligns with existing skips for -race, but in others
we've got new issues. These are debug modes, so some performance loss is
expected, and this is clearly no worse than today where the tests fail.
Next, skip internal linking and static linking tests for msan and asan.
With asan we get an explicit failure that neither are supported by the C
and/or Go compilers. With msan, we only get the Go compiler telling us
internal linking is unavailable. With static linking, we segfault
instead. Filed #70080 to track that.
Next, skip some malloc tests with asan that don't quite work because of
the redzone.
This is because of some sizeclass assumptions that get broken with the
redzone and the fact that the tiny allocator is effectively disabled
(again, due to the redzone).
Next, skip some runtime/pprof tests with asan, because of extra
allocations.
Next, skip some malloc tests with asan that also fail because of extra
allocations.
Next, fix up memstats accounting for arenas when asan is enabled. There
is a bug where more is added to the stats than subtracted. This also
simplifies the accounting a little.
Next, skip race tests with msan or asan enabled; they're mutually
incompatible.
Fixes#70054.
Fixes#64256.
Fixes#64257.
For #70079.
For #70080.
Change-Id: I99c02a0b9d621e44f1f918b307aa4a4944c3ec60
Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-asan-clang15,gotip-linux-amd64-msan-clang15
Reviewed-on: https://go-review.googlesource.com/c/go/+/622855
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Bypass: Michael Knyszek <mknyszek@google.com>
The current implementation has a panic when the database is closed
concurrently with a new connection attempt.
connRequestSet.CloseAndRemoveAll sets connRequestSet.s to a nil slice.
If this happens between calls to connRequestSet.Add and
connRequestSet.Delete, there is a panic when trying to write to the nil
slice. This is sequence is likely to occur in DB.conn, where the mutex
is released between calls to db.connRequests.Add and
db.connRequests.Delete
This change updates connRequestSet.CloseAndRemoveAll to set the curIdx
to -1 for all pending requests before setting its internal slice to nil.
CloseAndRemoveAll already iterates the full slice to close all the request
channels. It seems appropriate to set curIdx to -1 before deleting the
slice for 3 reasons:
1. connRequestSet.deleteIndex also sets curIdx to -1
2. curIdx will not be relevant to anything after the slice is set to nil
3. connRequestSet.Delete already checks for negative indices
Fixes#68949
Change-Id: I6b7ebc5a71b67322908271d13865fa12f2469b87
GitHub-Last-Rev: 7d2669155b
GitHub-Pull-Request: golang/go#68953
Reviewed-on: https://go-review.googlesource.com/c/go/+/607238
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Ian Lance Taylor <iant@golang.org>
Commit-Queue: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Add linknames for most modules with ≥50 dependents.
Add linknames for a few other modules that we know
are important but are below 50.
Remove linknames from badlinkname.go that do not merit
inclusion (very small number of dependents).
We can add them back later if the need arises.
Fixes#67401. (For now.)
Change-Id: I1e49fec0292265256044d64b1841d366c4106002
Reviewed-on: https://go-review.googlesource.com/c/go/+/587756
Auto-Submit: Russ Cox <rsc@golang.org>
TryBot-Bypass: Russ Cox <rsc@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
CL 585358 adds restrictions to disallow pull-only linknames
(currently off by default). Currently, there are quite some pull-
only linknames in user code in the wild. In order not to break
those, we add push linknames to allow them to be pulled. This CL
includes linknames found in a large code corpus (thanks Matthew
Dempsky and Michael Pratt for the analysis!), that are not
currently linknamed.
Updates #67401.
Change-Id: I32f5fc0c7a6abbd7a11359a025cfa2bf458fe767
Reviewed-on: https://go-review.googlesource.com/c/go/+/586137
Reviewed-by: Russ Cox <rsc@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Depending on the query, a RawBytes can contain memory owned by the
driver or by database/sql:
If the driver provides the column as a []byte,
RawBytes aliases that []byte.
If the driver provides the column as any other type,
RawBytes contains memory allocated by database/sql.
Prior to this CL, Rows.Scan will reuse existing capacity in a
RawBytes to permit a single allocation to be reused across rows.
When a RawBytes is reused across queries, this can result
in database/sql writing to driver-owned memory.
Add a buffer to Rows to store RawBytes data, and reuse this
buffer across calls to Rows.Scan.
Fixes#65201
Change-Id: Iac640174c7afa97eeb39496f47dec202501b2483
Reviewed-on: https://go-review.googlesource.com/c/go/+/557917
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This replaces a map used as a set with a slice.
We were using a surprising amount of CPU in this code, making mapiters
to pull out a random element of the map. Instead, just rand.IntN to pick
a random element of the slice.
It also adds a benchmark:
│ before │ after │
│ sec/op │ sec/op vs base │
ConnRequestSet-8 1818.0n ± 0% 452.4n ± 0% -75.12% (p=0.000 n=10)
(whether random is a good policy is a bigger question, but this
optimizes the current policy without changing behavior)
Updates #66361
Change-Id: I3d456a819cc720c2d18e1befffd2657e5f50f1e7
Reviewed-on: https://go-review.googlesource.com/c/go/+/572119
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
Reviewed-by: David Chase <drchase@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Brad Fitzpatrick <bradfitz@golang.org>
CL 497675 modified Rows such that context errors are propagated through
Rows.Err(). This caused an issue where calling Close meant that an
internal cancellation error would (eventually) be returned from Err:
1. A caller makes a query using a cancellable context.
2. initContextClose sees that either the query context or the
transaction context can be canceled, so will need to spawn a
goroutine to capture their errors.
3. initContextClose derives a context from the query context via
WithCancel and sets rs.cancel.
4. When a user calls Close, rs.cancel is called. awaitDone's ctx is
cancelled, which is good, since we don't want it to hang forever.
5. This internal cancellation (after CL 497675) has its error saved on
contextDone.
6. Later, calling Err will return the error in contextDone if present.
This leads to a race condition depending on how quickly Err is called
after Close.
The docs for Close and Err state that calling Close should have no
affect on the return result for Err. So, a potential fix is to ensure
that awaitDone does not save the error when the cancellation comes from
a Close via rs.cancel.
This CL does that, using a new context not derived from the query
context, whose error is ignored as the query context's error used to be
before the original bugfix.
The included test fails before the CL, and passes afterward.
Fixes#60932
Change-Id: I2bf4c549efd83d62b86e298c9c45ebd06a3ad89a
Reviewed-on: https://go-review.googlesource.com/c/go/+/505397
Auto-Submit: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
If the cancellation takes effect between Next and Scan,
then Scan returns context.Canceled, but the test wasn't
allowing this case.
The old flake reproduced easily with:
go test -c
stress ./sql.test -test.count=100 -test.run=TestContextCancelDuringRawBytesScan
The new test modes exercise that path directly instead of needing stress.
The new check for context.Canceled fixes the new test mode "top".
Fixes#60445.
Change-Id: I3870039a0fbe0a43c3e261b43b175ef83f818765
Reviewed-on: https://go-review.googlesource.com/c/go/+/502876
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Auto-Submit: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
The earlier CL 497675 for #60304 introduced a behavior change
that, while not strictly a bug, caused a bunch of test failures
in a large codebase. Rather than add behavior changes in a 10 year
old package, revert to the old behavior: a context cancelation
between Rows.Next reporting false and a call to Rows.Err should
not result in Rows.Err returning the context error.
That behavior was accidentally added in CL 497675 as part of changing
how contexts and Rows iteration worked.
Updates #60304
Updates #53970
Change-Id: I22f8a6a6b0b5a94b430576cf50e015efd01ec652
Reviewed-on: https://go-review.googlesource.com/c/go/+/498398
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
sql.RawBytes was added the very first Go release, Go 1. Its docs
say:
> RawBytes is a byte slice that holds a reference to memory owned by
> the database itself. After a Scan into a RawBytes, the slice is only
> valid until the next call to Next, Scan, or Close.
That "only valid until the next call" bit was true at the time,
until contexts were added to database/sql in Go 1.8.
In the past ~dozen releases it's been unsafe to use QueryContext with
a context that might become Done to get an *sql.Rows that's scanning
into a RawBytes. The Scan can succeed, but then while the caller's
reading the memory, a database/sql-managed goroutine can see the
context becoming done and call Close on the database/sql/driver and
make the caller's view of the RawBytes memory no longer valid,
introducing races, crashes, or database corruption. See #60304
and #53970 for details.
This change does the minimal surgery on database/sql to make it safe
again: Rows.Scan was already acquiring a mutex to check whether the
rows had been closed, so this change make Rows.Scan notice whether
*RawBytes was used and, if so, doesn't release the mutex on exit
before returning. That mean it's still locked while the user code
operates on the RawBytes memory and the concurrent context-watching
goroutine to close the database still runs, but if it fires, it then
gets blocked on the mutex until the next call to a Rows method (Next,
NextResultSet, Err, Close).
Updates #60304
Updates #53970 (earlier one I'd missed)
Change-Id: Ie41c0c6f32c24887b2f53ec3686c2aab73a1bfff
Reviewed-on: https://go-review.googlesource.com/c/go/+/497675
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
This removes an allocation in Conn.grabConn that, while not super
important, was distracting me when optimizing code elsewhere.
While here, convert an atomic that was forgotten when this package was
earlier updated to use the new Go 1.19 typed atomics.
Change-Id: I4666256b4c0512e2162bd485c389130699f9d5ed
Reviewed-on: https://go-review.googlesource.com/c/go/+/475415
Reviewed-by: David Crawshaw <crawshaw@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
