Skip to content

fix(logservice): create on-fly clients outside pool lock - #25440

Merged
mergify[bot] merged 3 commits into
matrixorigin:4.1-devfrom
jiangxinmeng1:cherry-pick-pr-25290-4.1-dev
Jul 4, 2026
Merged

fix(logservice): create on-fly clients outside pool lock#25440
mergify[bot] merged 3 commits into
matrixorigin:4.1-devfrom
jiangxinmeng1:cherry-pick-pr-25290-4.1-dev

Conversation

@jiangxinmeng1

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #25275

What this PR does / why we need it:

This PR prevents clientPool.GetOnFly from holding the pool mutex while creating a new logservice client.

Previously, GetOnFly held c.cond.L during NewClient, which may perform network I/O and block for a long time when logservice replicas are unreachable. While that lock was held, normal pool operations such as Get, Put,
and GetWithWriteToken could not proceed, causing the WAL commit path to stall.

Changes:

  • Move NewClient execution outside the clientPool mutex.
  • Keep the lock only for checking whether the pool is closed.
  • Re-check the closed state after client creation.
  • Close the newly created client and return ErrClientPoolClosed if the pool was closed during creation.
  • Add a regression test proving that pool Get is not blocked while GetOnFly is creating a client.

@jiangxinmeng1
jiangxinmeng1 requested a review from XuPeng-SH as a code owner July 3, 2026 08:42
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@mergify mergify Bot added the kind/bug Something isn't working label Jul 3, 2026
@mergify mergify Bot added the queued label Jul 4, 2026
@mergify

mergify Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Merge Queue Status

  • Entered queue2026-07-04 08:04 UTC · Rule: release-4.1 · triggered by rule Automatic queue on approval for release-4.1
  • Checks skipped · PR is already up-to-date
  • Merged2026-07-04 08:04 UTC · at da9b6d5b79017927308d61c63aec6382ce5c45ad · squash

This pull request spent 11 seconds in the queue, including 1 second running CI.

Required conditions to merge
  • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
  • github-review-approved [🛡 GitHub branch protection]
  • github-review-decision = APPROVED [🛡 GitHub branch protection]
  • any of [🛡 GitHub branch protection]:
    • check-success = Matrixone CI / SCA Test on Linux/arm64
    • check-neutral = Matrixone CI / SCA Test on Linux/arm64
    • check-skipped = Matrixone CI / SCA Test on Linux/arm64
  • any of [🛡 GitHub branch protection]:
    • check-success = Matrixone CI / UT Test on Ubuntu/x86
    • check-neutral = Matrixone CI / UT Test on Ubuntu/x86
    • check-skipped = Matrixone CI / UT Test on Ubuntu/x86
  • any of [🛡 GitHub branch protection]:
    • check-success = Matrixone Compose CI / multi cn e2e bvt test docker compose(Optimistic/PUSH)
    • check-neutral = Matrixone Compose CI / multi cn e2e bvt test docker compose(Optimistic/PUSH)
    • check-skipped = Matrixone Compose CI / multi cn e2e bvt test docker compose(Optimistic/PUSH)
  • any of [🛡 GitHub branch protection]:
    • check-success = Matrixone Compose CI / multi cn e2e bvt test docker compose(PESSIMISTIC)
    • check-neutral = Matrixone Compose CI / multi cn e2e bvt test docker compose(PESSIMISTIC)
    • check-skipped = Matrixone Compose CI / multi cn e2e bvt test docker compose(PESSIMISTIC)
  • any of [🛡 GitHub branch protection]:
    • check-success = Matrixone Standlone CI / Multi-CN e2e BVT Test on Linux/x64(LAUNCH, PROXY)
    • check-neutral = Matrixone Standlone CI / Multi-CN e2e BVT Test on Linux/x64(LAUNCH, PROXY)
    • check-skipped = Matrixone Standlone CI / Multi-CN e2e BVT Test on Linux/x64(LAUNCH, PROXY)
  • any of [🛡 GitHub branch protection]:
    • check-success = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH, PESSIMISTIC)
    • check-neutral = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH, PESSIMISTIC)
    • check-skipped = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH, PESSIMISTIC)
  • any of [🛡 GitHub branch protection]:
    • check-success = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH,Optimistic)
    • check-neutral = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH,Optimistic)
    • check-skipped = Matrixone Standlone CI / e2e BVT Test on Linux/x64(LAUNCH,Optimistic)
  • any of [🛡 GitHub branch protection]:
    • check-success = Matrixone Utils CI / Coverage
    • check-neutral = Matrixone Utils CI / Coverage
    • check-skipped = Matrixone Utils CI / Coverage
  • any of [🛡 GitHub branch protection]:
    • check-success = Matrixone Upgrade CI / Compatibility Test With Target on Linux/x64(LAUNCH)
    • check-neutral = Matrixone Upgrade CI / Compatibility Test With Target on Linux/x64(LAUNCH)
    • check-skipped = Matrixone Upgrade CI / Compatibility Test With Target on Linux/x64(LAUNCH)

@mergify
mergify Bot merged commit b03babe into matrixorigin:4.1-dev Jul 4, 2026
23 of 24 checks passed
@mergify mergify Bot removed the queued label Jul 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working size/M Denotes a PR that changes [100,499] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants