Skip to content

Conversation

@timfn-hg
Copy link
Contributor

Description:
These changes try to improve handling of concurrent close operations on a block node connection and the almost unavoidable race conditions associated with it. A new state (CLOSING) is introduced to signal that a connection is in the process of closing and more fine grain checks and controls related to closing are implemented.

Additionally, a new check is added when sending a request to a block node such that if/when a race condition is encountered when a connection is being closed by one thread while another thread is trying to send a request over the same connection, we will ignore any errors that happen if the connection is not in an active state.

Also introduced is a connection ID that is unique to each connection instance. This makes it much easier to group together logs for an individual connection instance, especially when one or more connections are inflight to the same host (e.g. testing locally with multiple BN connections all to localhost.)

Related issue(s):

Fixes #21241

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@timfn-hg timfn-hg added this to the v0.68 milestone Sep 25, 2025
@timfn-hg timfn-hg self-assigned this Sep 25, 2025
@timfn-hg timfn-hg requested review from a team as code owners September 25, 2025 19:30
@lfdt-bot
Copy link

lfdt-bot commented Sep 25, 2025

🎉 Snyk checks have passed. No issues have been found so far.

security/snyk check is complete. No issues have been found. (View Details)

@codacy-production
Copy link

codacy-production bot commented Sep 25, 2025

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
-0.03% (target: -1.00%) 79.49%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (335fb2c) 103366 77740 75.21%
Head commit (fb14a42) 103406 (+40) 77736 (-4) 75.18% (-0.03%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#21247) 78 62 79.49%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

@codecov
Copy link

codecov bot commented Sep 25, 2025

Codecov Report

❌ Patch coverage is 70.51282% with 23 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...app/blocks/impl/streaming/BlockNodeConnection.java 70.49% 11 Missing and 7 partials ⚠️
...cks/impl/streaming/BlockNodeConnectionManager.java 70.58% 5 Missing ⚠️

Impacted file tree graph

@@             Coverage Diff              @@
##               main   #21247      +/-   ##
============================================
- Coverage     71.27%   71.04%   -0.24%     
- Complexity    24229    24237       +8     
============================================
  Files          2669     2673       +4     
  Lines        103461   103812     +351     
  Branches      10771    10803      +32     
============================================
+ Hits          73742    73753      +11     
- Misses        25675    26014     +339     
- Partials       4044     4045       +1     
Files with missing lines Coverage Δ Complexity Δ
...cks/impl/streaming/BlockNodeConnectionManager.java 85.85% <70.58%> (+0.07%) 65.00 <11.00> (+2.00)
...app/blocks/impl/streaming/BlockNodeConnection.java 77.45% <70.49%> (+0.53%) 51.00 <10.00> (+4.00)

... and 44 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Tim Farber-Newman <[email protected]>
Signed-off-by: Tim Farber-Newman <[email protected]>
Copy link
Contributor

@derektriley derektriley left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @timfn-hg

@timfn-hg timfn-hg merged commit 1c7ad75 into main Sep 26, 2025
51 of 52 checks passed
@timfn-hg timfn-hg deleted the timfn/21241-improve-bn-conn-close branch September 26, 2025 20:44
@petreze petreze restored the timfn/21241-improve-bn-conn-close branch September 29, 2025 08:01
@petreze petreze deleted the timfn/21241-improve-bn-conn-close branch September 29, 2025 08:06
mxtartaglia-sl pushed a commit that referenced this pull request Nov 3, 2025
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.

Improve concurrent block node connection closing

4 participants