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.

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

Two way streams fixes - take 2 #19796

Merged
merged 11 commits into from
Jun 7, 2024
Merged

Conversation

klauspost
Copy link
Contributor

@klauspost klauspost commented May 23, 2024

Description

Re-add #19763 - fixes are individual commits.

Fixes races observed with new tests and tweak tests for flaky operations.

Also fixes a goroutine leak with (unused) two-way streams.

Motivation and Context

How to test this PR?

In internal/grid, use go test -v --count=100 -race -timeout=30m -failfast or similar.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Unit tests added/updated

Calling `disconnect with locked `respmu` would cause a deadlock, since it calls `close` - which also attempts to get the lock.

Also fixes leak in `handleInbound` with a canceled server.
@klauspost
Copy link
Contributor Author

@krisis @vadmeste PTAL. Now (just post release) would be a good time to get this merged.

internal/grid/connection.go Outdated Show resolved Hide resolved
Copy link
Member

@krisis krisis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Out of curiosity, where would two way streams be used in MinIO's internode communications?

@klauspost
Copy link
Contributor Author

@krisis Locking is most obvious. That way the request will track the lifetime of the lock request to the remote.

@harshavardhana harshavardhana merged commit f001870 into minio:master Jun 7, 2024
20 checks passed
@klauspost klauspost deleted the two-way-streams2 branch June 20, 2024 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants