Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Data race between mysqlConn.cleanup() and writeHandshakeResponsePacket #1567

Closed
bobvawter opened this issue Mar 19, 2024 · 2 comments · Fixed by #1570
Closed

Data race between mysqlConn.cleanup() and writeHandshakeResponsePacket #1567

bobvawter opened this issue Mar 19, 2024 · 2 comments · Fixed by #1570

Comments

@bobvawter
Copy link

Issue description

This is a follow-on from #1559 that popped out of our CI system running the 1.8.1 pre-release.

Error log

==================
WARNING: DATA RACE
Read at 0x00c000205e78 by goroutine 29179:
  github.com/go-sql-driver/mysql.(*mysqlConn).cleanup()
      /home/runner/go/pkg/mod/github.com/go-sql-driver/mysql@v1.8.1-0.20240317050433-65395d853d2c/connection.go:155 +0x86
  github.com/go-sql-driver/mysql.(*mysqlConn).cancel()
      /home/runner/go/pkg/mod/github.com/go-sql-driver/mysql@v1.8.1-0.20240317050433-65395d853d2c/connection.go:449 +0x84
  github.com/go-sql-driver/mysql.(*mysqlConn).startWatcher.func1()
      /home/runner/go/pkg/mod/github.com/go-sql-driver/mysql@v1.8.1-0.20240317050433-65395d853d2c/connection.go:636 +0x234

Previous write at 0x00c000205e78 by goroutine 29055:
  github.com/go-sql-driver/mysql.(*mysqlConn).writeHandshakeResponsePacket()
      /home/runner/go/pkg/mod/github.com/go-sql-driver/mysql@v1.8.1-0.20240317050433-[653](https://github.com/cockroachdb/cdc-sink/actions/runs/8350784301/job/22857940722?pr=767#step:6:654)95d853d2c/packets.go:355 +0x11e4
  github.com/go-sql-driver/mysql.(*connector).Connect()
      /home/runner/go/pkg/mod/github.com/go-sql-driver/mysql@v1.8.1-0.20240317050433-65395d853d2c/connector.go:154 +0xf3a
  database/sql.(*DB).conn()
      /opt/hostedtoolcache/go/1.20.14/x64/src/database/sql/sql.go:1387 +0xb0f
  database/sql.(*DB).begin()
      /opt/hostedtoolcache/go/1.20.14/x64/src/database/sql/sql.go:1853 +0x70
  database/sql.(*DB).BeginTx.func1()
      /opt/hostedtoolcache/go/1.20.14/x64/src/database/sql/sql.go:1836 +0x8a
  database/sql.(*DB).retry()
      /opt/hostedtoolcache/go/1.20.14/x64/src/database/sql/sql.go:1538 +0x4c
  database/sql.(*DB).BeginTx()
      /opt/hostedtoolcache/go/1.20.14/x64/src/database/sql/sql.go:1835 +0xc9
  github.com/cockroachdb/cdc-sink/internal/sequencer/core.(*round).tryCommit()
      /home/runner/work/cdc-sink/cdc-sink/internal/sequencer/core/round.go:150 +0x446
  github.com/cockroachdb/cdc-sink/internal/sequencer/core.(*round).scheduleCommit.func1.2()
      /home/runner/work/cdc-sink/cdc-sink/internal/sequencer/core/round.go:103 +0x58
  github.com/cockroachdb/cdc-sink/internal/util/retry.Retry.func1()
      /home/runner/work/cdc-sink/cdc-sink/internal/util/retry/retry.go:67 +0x58
  github.com/cockroachdb/cdc-sink/internal/util/retry.Loop()
      /home/runner/work/cdc-sink/cdc-sink/internal/util/retry/retry.go:94 +0x210
  github.com/cockroachdb/cdc-sink/internal/util/retry.Retry()
      /home/runner/work/cdc-sink/cdc-sink/internal/util/retry/retry.go:66 +0xa8
  github.com/cockroachdb/cdc-sink/internal/sequencer/core.(*round).scheduleCommit.func1()
      /home/runner/work/cdc-sink/cdc-sink/internal/sequencer/core/round.go:102 +0x1da
  github.com/cockroachdb/cdc-sink/internal/sequencer/scheduler.(*Scheduler).Batch.func1()
      /home/runner/work/cdc-sink/cdc-sink/internal/sequencer/scheduler/scheduler.go:45 +0x164
  github.com/cockroachdb/cdc-sink/internal/util/lockset.tryCall[...]()
      /home/runner/work/cdc-sink/cdc-sink/internal/util/lockset/lockset.go:511 +0x141
  github.com/cockroachdb/cdc-sink/internal/util/lockset.(*Set[...]).dispose.func1()
      /home/runner/work/cdc-sink/cdc-sink/internal/util/lockset/lockset.go:352 +0x20d
  github.com/cockroachdb/cdc-sink/internal/util/workgroup.(*Group).worker()
      /home/runner/work/cdc-sink/cdc-sink/internal/util/workgroup/workgroup.go:150 +0x4c6
  github.com/cockroachdb/cdc-sink/internal/util/workgroup.(*Group).maybeStart.func2()
      /home/runner/work/cdc-sink/cdc-sink/internal/util/workgroup/workgroup.go:114 +0x64

Goroutine 29179 (running) created at:
  github.com/go-sql-driver/mysql.(*mysqlConn).startWatcher()
      /home/runner/go/pkg/mod/github.com/go-sql-driver/mysql@v1.8.1-0.20240317050433-65395d853d2c/connection.go:625 +0x1c4
  github.com/go-sql-driver/mysql.(*connector).Connect()
      /home/runner/go/pkg/mod/github.com/go-sql-driver/mysql@v1.8.1-0.20240317050433-65395d853d2c/connector.go:118 +0xa04
  database/sql.(*DB).conn()
      /opt/hostedtoolcache/go/1.20.14/x64/src/database/sql/sql.go:1387 +0xb0f
  database/sql.(*DB).begin()
      /opt/hostedtoolcache/go/1.20.14/x64/src/database/sql/sql.go:1853 +0x70
  database/sql.(*DB).BeginTx.func1()
      /opt/hostedtoolcache/go/1.20.14/x64/src/database/sql/sql.go:1836 +0x8a
  database/sql.(*DB).retry()
      /opt/hostedtoolcache/go/1.20.14/x64/src/database/sql/sql.go:1538 +0x4c
  database/sql.(*DB).BeginTx()
      /opt/hostedtoolcache/go/1.20.14/x64/src/database/sql/sql.go:1835 +0xc9
  github.com/cockroachdb/cdc-sink/internal/sequencer/core.(*round).tryCommit()
      /home/runner/work/cdc-sink/cdc-sink/internal/sequencer/core/round.go:150 +0x446
  github.com/cockroachdb/cdc-sink/internal/sequencer/core.(*round).scheduleCommit.func1.2()
      /home/runner/work/cdc-sink/cdc-sink/internal/sequencer/core/round.go:103 +0x58
  github.com/cockroachdb/cdc-sink/internal/util/retry.Retry.func1()
      /home/runner/work/cdc-sink/cdc-sink/internal/util/retry/retry.go:67 +0x58
  github.com/cockroachdb/cdc-sink/internal/util/retry.Loop()
      /home/runner/work/cdc-sink/cdc-sink/internal/util/retry/retry.go:94 +0x210
  github.com/cockroachdb/cdc-sink/internal/util/retry.Retry()
      /home/runner/work/cdc-sink/cdc-sink/internal/util/retry/retry.go:66 +0xa8
  github.com/cockroachdb/cdc-sink/internal/sequencer/core.(*round).scheduleCommit.func1()
      /home/runner/work/cdc-sink/cdc-sink/internal/sequencer/core/round.go:102 +0x1da
  github.com/cockroachdb/cdc-sink/internal/sequencer/scheduler.(*Scheduler).Batch.func1()
      /home/runner/work/cdc-sink/cdc-sink/internal/sequencer/scheduler/scheduler.go:45 +0x164
  github.com/cockroachdb/cdc-sink/internal/util/lockset.tryCall[...]()
      /home/runner/work/cdc-sink/cdc-sink/internal/util/lockset/lockset.go:511 +0x141
  github.com/cockroachdb/cdc-sink/internal/util/lockset.(*Set[...]).dispose.func1()
      /home/runner/work/cdc-sink/cdc-sink/internal/util/lockset/lockset.go:352 +0x20d
  github.com/cockroachdb/cdc-sink/internal/util/workgroup.(*Group).worker()
      /home/runner/work/cdc-sink/cdc-sink/internal/util/workgroup/workgroup.go:150 +0x4c6
  github.com/cockroachdb/cdc-sink/internal/util/workgroup.(*Group).maybeStart.func2()
      /home/runner/work/cdc-sink/cdc-sink/internal/util/workgroup/workgroup.go:114 +0x64

Goroutine 29055 (running) created at:
  github.com/cockroachdb/cdc-sink/internal/util/workgroup.(*Group).maybeStart()
      /home/runner/work/cdc-sink/cdc-sink/internal/util/workgroup/workgroup.go:114 +0x2c5
  github.com/cockroachdb/cdc-sink/internal/util/workgroup.(*Group).Go()
      /home/runner/work/cdc-sink/cdc-sink/internal/util/workgroup/workgroup.go:79 +0x12f
  github.com/cockroachdb/cdc-sink/internal/util/lockset.(*Set[...]).dispose()
      /home/runner/work/cdc-sink/cdc-sink/internal/util/lockset/lockset.go:416 +0x1fb
  github.com/cockroachdb/cdc-sink/internal/util/lockset.(*Set[...]).enqueue()
      /home/runner/work/cdc-sink/cdc-sink/internal/util/lockset/lockset.go:452 +0x6f5
  github.com/cockroachdb/cdc-sink/internal/util/lockset.(*Set[...]).Schedule()
      /home/runner/work/cdc-sink/cdc-sink/internal/util/lockset/lockset.go:216 +0x244
  github.com/cockroachdb/cdc-sink/internal/sequencer/scheduler.(*Scheduler).Batch()
      /home/runner/work/cdc-sink/cdc-sink/internal/sequencer/scheduler/scheduler.go:39 +0x12b
  github.com/cockroachdb/cdc-sink/internal/sequencer/core.(*round).scheduleCommit()
      /home/runner/work/cdc-sink/cdc-sink/internal/sequencer/core/round.go:84 +0x23c
  github.com/cockroachdb/cdc-sink/internal/sequencer/core.(*Core).Start.func1.4()
      /home/runner/work/cdc-sink/cdc-sink/internal/sequencer/core/core.go:181 +0x1d8
  github.com/cockroachdb/cdc-sink/internal/sequencer/core.(*Core).Start.func1.6()
      /home/runner/work/cdc-sink/cdc-sink/internal/sequencer/core/core.go:257 +0x25c
  github.com/cockroachdb/cdc-sink/internal/sequencer/sequtil.(*Copier).Run()
      /home/runner/work/cdc-sink/cdc-sink/internal/sequencer/sequtil/copier.go:73 +0x22e
  github.com/cockroachdb/cdc-sink/internal/sequencer/core.(*Core).Start.func1.8()
      /home/runner/work/cdc-sink/cdc-sink/internal/sequencer/core/core.go:271 +0x5c
  github.com/cockroachdb/cdc-sink/internal/util/stopper.(*Context).Go.func1()
      /home/runner/work/cdc-sink/cdc-sink/internal/util/stopper/stopper.go:187 +0xb4
==================

Configuration

Driver version (or git SHA): v1.8.1-0.20240317050433-65395d853d2c

Go version: go 1.20.14

Server version: MySQL v8

Server OS: GitHub ubuntu-latest runner.
@bobvawter bobvawter mentioned this issue Mar 19, 2024
Merged
bobvawter added a commit to cockroachdb/replicator that referenced this issue Mar 20, 2024
@bobvawter
workflow: Disable race detector for mysql
98158ec 
Revert when go-sql-driver/mysql#1567 is closed
github-merge-queue bot pushed a commit to cockroachdb/replicator that referenced this issue Mar 20, 2024
@bobvawter
workflow: Disable race detector for mysql
bfaebd1 
Revert when go-sql-driver/mysql#1567 is closed
shogo82148 added a commit to shogo82148/mysql that referenced this issue Mar 22, 2024
@shogo82148
fix Data race between mysqlConn.cleanup() and writeHandshakeResponseP…
93f1a64 
…acket

close go-sql-driver#1567
@shogo82148 shogo82148 mentioned this issue Mar 22, 2024
Merged
5 tasks
shogo82148 added a commit that referenced this issue Mar 22, 2024
@shogo82148
Fix issue 1567 (#1570)
d7ddb8b 
### Description

closes #1567

When TLS is enabled, `mc.netConn` is rewritten after the TLS handshak as
detailed here:


https://github.com/go-sql-driver/mysql/blob/d86c4527bae98ccd4e5060f72887520ce30eda5e/packets.go#L355

Therefore, `mc.netConn` should not be accessed within the watcher
goroutine.
Instead, `mc.rawConn` should be initialized prior to invoking
`mc.startWatcher`, and `mc.rawConn` should be used in lieu of
`mc.netConn`.

### Checklist
- [x] Code compiles correctly
- [x] Created tests which fail without the change (if possible)
- [x] All tests passing
- [x] Extended the README / documentation, if necessary
- [x] Added myself / the copyright holder to the AUTHORS file


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

- **Refactor**
	- Improved variable naming for better code readability and maintenance.
	- Enhanced network connection handling logic.
- **New Features**
	- Updated TCP connection handling to better support TCP Keepalives.
- **Tests**
- Added a new test to address and verify the fix for a specific issue
related to TLS, connection pooling, and round trip time estimation.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
@shogo82148
Copy link
Contributor

Could you try 7eeaba6 ?

@bobvawter
Copy link
Author

The 7eeaba6 commit looks stable in our CI environment. Thanks.

@shogo82148 shogo82148 mentioned this issue Mar 26, 2024
Merged
5 tasks
shogo82148 added a commit that referenced this issue Mar 26, 2024
@shogo82148
update changelog for releasing v1.8.1 (#1576)
4395c45 
### Description

#1559 and
#1567 are fixed.
Let's release a new version v1.8.1.

### Checklist
- [x] Code compiles correctly
- [x] Created tests which fail without the change (if possible)
- [x] All tests passing
- [x] Extended the README / documentation, if necessary
- [x] Added myself / the copyright holder to the AUTHORS file


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

- **Bug Fixes**
	- Addressed race condition issues for enhanced stability.

- **New Features**
- Improved database compatibility with charset and collation
adjustments.
- Enhanced security and flexibility through the introduction of new
configuration options.

- **Major Changes**
- Dropped support for older versions of Go (1.13-1.17) to leverage newer
language features.
	- Improved number parsing for efficiency and accuracy.
	- Added configurable logging per connection for better diagnostics.

- **Enhancements**
- Fixed issues with ColumnType.DatabaseTypeName to improve data
handling.
- Introduced connection attributes for more detailed connection
information.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
shogo82148 added a commit to shogo82148/mysql that referenced this issue Mar 26, 2024
@shogo82148
update changelog for releasing v1.8.1 (go-sql-driver#1576)
476df92 
### Description

go-sql-driver#1559 and
go-sql-driver#1567 are fixed.
Let's release a new version v1.8.1.

### Checklist
- [x] Code compiles correctly
- [x] Created tests which fail without the change (if possible)
- [x] All tests passing
- [x] Extended the README / documentation, if necessary
- [x] Added myself / the copyright holder to the AUTHORS file


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

- **Bug Fixes**
	- Addressed race condition issues for enhanced stability.

- **New Features**
- Improved database compatibility with charset and collation
adjustments.
- Enhanced security and flexibility through the introduction of new
configuration options.

- **Major Changes**
- Dropped support for older versions of Go (1.13-1.17) to leverage newer
language features.
	- Improved number parsing for efficiency and accuracy.
	- Added configurable logging per connection for better diagnostics.

- **Enhancements**
- Fixed issues with ColumnType.DatabaseTypeName to improve data
handling.
- Introduced connection attributes for more detailed connection
information.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix issue 1567 shogo82148/mysql
2 participants
@shogo82148 @bobvawter