Skip to content

Update Stats to use atomic operations#54

Merged
mateeullahmalik merged 1 commit intomasterfrom
bugfix/conc-handshake-testutil
May 14, 2025
Merged

Update Stats to use atomic operations#54
mateeullahmalik merged 1 commit intomasterfrom
bugfix/conc-handshake-testutil

Conversation

@mateeullahmalik
Copy link
Collaborator

@mateeullahmalik mateeullahmalik commented May 12, 2025

Issue

The TestHandshakerConcurrentHandshakes test fails with a race condition when testing with maximum concurrent handshakes:

fatal error: concurrent map read and map write
goroutine 559 [running]:
internal/runtime/maps.fatal({...})
github.com/99designs/keyring.(*ArrayKeyring).Get(...)
github.com/cosmos/cosmos-sdk/crypto/keyring.keystore.KeyByAddress(...) 
github.com/LumeraProtocol/lumera/x/lumeraid/securekeyx.(*SecureKeyExchange).getLocalPubKey(...)
github.com/LumeraProtocol/lumera/x/lumeraid/securekeyx.(*SecureKeyExchange).CreateRequest(...)

Changes

  • Replaced mutex locks with atomic operations for thread-safe access
  • Changed field types from int to int32 to support atomic operations
  • Added a GetMaxConcurrentCalls() accessor method for safe value retrieval
  • Updated the relevant tests to use the new accessor method

The update preserves the original API contract where Update() still returns a cleanup function that decrements the call count when invoked. These changes should reduce overhead in high-concurrency scenarios.

@a-ok123 a-ok123 requested review from akobrin1 and Copilot May 12, 2025 16:44
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses a race condition occurring during concurrent ALTS handshake tests by introducing mutex protection in SecureKeyExchange and converting Stats to use atomic primitives for thread safety.

  • Introduced atomic operations for call counting and tracking maximum concurrent calls in Stats.
  • Updated test assertions to call the new MaxConcurrentCalls() method.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
pkg/net/credentials/alts/testutil/testutil.go Replaced mutex with atomic operations for improved concurrency safety in Stats.
pkg/net/credentials/alts/handshake/handshake_test.go Updated test assertions to utilize the new MaxConcurrentCalls() method.

Copy link
Contributor

@akobrin1 akobrin1 left a comment

Choose a reason for hiding this comment

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

we can change implementation of Stats to atomic, but we still need Update method to return cleanup function - to correctly analyze that all handshake calls were completed.

@mateeullahmalik mateeullahmalik changed the title Fix concurrent map access in ALTS handshake tests Convert Stats to use atomic operations May 13, 2025
@mateeullahmalik mateeullahmalik force-pushed the bugfix/conc-handshake-testutil branch from e78afb9 to c6e648e Compare May 13, 2025 09:12
@mateeullahmalik mateeullahmalik requested a review from akobrin1 May 13, 2025 09:14
@mateeullahmalik mateeullahmalik changed the title Convert Stats to use atomic operations Update Stats to use atomic operations May 13, 2025
@mateeullahmalik mateeullahmalik merged commit 98cfd06 into master May 14, 2025
3 checks passed
@mateeullahmalik mateeullahmalik deleted the bugfix/conc-handshake-testutil branch May 14, 2025 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants