Skip to content

⚡ perf: Narrow client user hook lock scope safely#4375

Merged
ReneWerner87 merged 4 commits into
mainfrom
copilot/narrow-client-hook-lock-scope
May 29, 2026
Merged

⚡ perf: Narrow client user hook lock scope safely#4375
ReneWerner87 merged 4 commits into
mainfrom
copilot/narrow-client-hook-lock-scope

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 28, 2026

Description

Client request/response hooks were executed while holding the client write lock, forcing concurrent requests on a shared client to serialize through hook execution. This change narrows the lock scope safely by cloning user hook slices under lock, then releasing the lock before iterating those cloned user hooks, while keeping builtin hooks protected because they still touch shared fasthttp-backed client state during request/response preparation.

  • Lock scope

    • Clone userRequestHooks and userResponseHooks under lock with slices.Clone(...).
    • Release the lock before invoking cloned user hooks so concurrent requests are no longer serialized on user hook execution.
    • Keep builtin hook execution protected by the client lock to avoid races on shared internal client state.
    • Preserve existing execution order: user request hooks before builtin request hooks, builtin response hooks before user response hooks.
  • Concurrency coverage

    • Keep the focused tests proving concurrent preHooks and afterHooks executions are not blocked on user hook execution.
    • Re-run the affected client tests with -race to verify the slice-sharing race is removed.
func (c *core) preHooks() error {
	c.client.mu.RLock()
	userHooks := slices.Clone(c.client.userRequestHooks)
	c.client.mu.RUnlock()

	for _, f := range userHooks {
		if err := f(c.client, c.req); err != nil {
			return err
		}
	}

	c.client.mu.Lock()
	defer c.client.mu.Unlock()

	for _, f := range c.client.builtinRequestHooks {
		if err := f(c.client, c.req); err != nil {
			return err
		}
	}

	return nil
}

Changes introduced

  • Benchmarks: No benchmark changes in this PR.
  • Documentation Update: No docs changes; behavior change is internal to client hook execution.
  • Changelog/What's New: User client hooks no longer hold the client lock during execution, and user hook slices are cloned under lock to avoid races with concurrent hook registration.
  • Migration Guide: No migration required.
  • API Alignment with Express: Not applicable.
  • API Longevity: Preserves the existing hook API and execution order while making concurrent hook execution safer.
  • Examples: The added tests demonstrate concurrent hook execution and race-safe user hook iteration.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • Enhancement (improvement to existing features and functionality)
  • Documentation update (changes to documentation)
  • Performance improvement (non-breaking change which improves efficiency)
  • Code consistency (non-breaking change which improves code reliability and robustness)

Checklist

Before you submit your pull request, please make sure you meet these requirements:

  • Followed the inspiration of the Express.js framework for new functionalities, making them similar in usage.
  • Conducted a self-review of the code and provided comments for complex or critical parts.
  • Updated the documentation in the /docs/ directory for Fiber's documentation.
  • Added or updated unit tests to validate the effectiveness of the changes or new features.
  • Ensured that new and existing unit tests pass locally with the changes.
  • Verified that any new dependencies are essential and have been agreed upon by the maintainers/community.
  • Aimed for optimal performance with minimal allocations in the new code.
  • Provided benchmarks for the new code to analyze and improve upon.

Commit formatting

Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md

Copilot AI changed the title [WIP] Fix client hook lock scope to avoid serializing concurrent requests Narrow client hook lock scope to avoid serializing concurrent requests May 28, 2026
Copilot finished work on behalf of gaby May 28, 2026 04:57
Copilot AI requested a review from gaby May 28, 2026 04:57
@ReneWerner87 ReneWerner87 added this to v3 May 28, 2026
@ReneWerner87 ReneWerner87 added this to the v3 milestone May 28, 2026
@gaby gaby changed the title Narrow client hook lock scope to avoid serializing concurrent requests ⚡ perf: Narrow client hook lock scope to avoid serializing concurrent requests May 28, 2026
@gaby gaby marked this pull request as ready for review May 28, 2026 04:58
@gaby gaby requested a review from a team as a code owner May 28, 2026 04:58
Copy link
Copy Markdown
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 improves Fiber’s HTTP client concurrency by reducing lock contention in the request/response hook pipeline, allowing multiple requests using the same client to execute hook logic concurrently.

Changes:

  • Narrow Client.mu scope in preHooks/afterHooks to only reading hook slice references (instead of holding the lock during hook execution).
  • Add tests asserting that preHooks and afterHooks do not serialize concurrent executions behind the client lock.

Reviewed changes

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

File Description
client/core.go Switch hook execution to use a short RLock window rather than holding the client lock while running hooks.
client/core_test.go Add concurrency-focused tests to verify hook execution isn’t serialized by the client lock.

Comment thread client/core.go
Comment thread client/core.go Outdated
Copilot finished work on behalf of gaby May 28, 2026 12:16
Copilot AI requested a review from gaby May 28, 2026 12:16
@codecov
Copy link
Copy Markdown

codecov Bot commented May 28, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.33%. Comparing base (e5e93e6) to head (db64eca).
⚠️ Report is 28 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4375      +/-   ##
==========================================
- Coverage   91.39%   91.33%   -0.06%     
==========================================
  Files         132      132              
  Lines       13098    13113      +15     
==========================================
+ Hits        11971    11977       +6     
- Misses        710      717       +7     
- Partials      417      419       +2     
Flag Coverage Δ
unittests 91.33% <100.00%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

@ReneWerner87
Copy link
Copy Markdown
Member

@copilot The lock-narrowing looks good and CI is green, but one issue from the automated review still needs to be fixed before this is safe to merge.

Snapshotting the pre/response hook slices by plain assignment only copies the slice header; the backing array stays shared. A concurrent AddRequestHook / AddResponseHook (which appends under c.mu) can mutate that backing array in place while this goroutine iterates the snapshot after the unlock, which is still a data race.

Please clone the slices while holding the lock, then release and iterate the clones:

reqHooks := slices.Clone(c.userRequestHooks)
resHooks := slices.Clone(c.userResponseHooks)

Re-run the affected tests with -race to confirm the race is gone.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 29, 2026

@copilot The lock-narrowing looks good and CI is green, but one issue from the automated review still needs to be fixed before this is safe to merge.

Snapshotting the pre/response hook slices by plai...

Addressed in db64eca.

Copilot AI changed the title ⚡ perf: Narrow client hook lock scope to avoid serializing concurrent requests ⚡ perf: Narrow client user hook lock scope safely May 29, 2026
@ReneWerner87 ReneWerner87 merged commit ee59a3b into main May 29, 2026
20 checks passed
@ReneWerner87 ReneWerner87 deleted the copilot/narrow-client-hook-lock-scope branch May 29, 2026 13:41
@github-project-automation github-project-automation Bot moved this to Done in v3 May 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

🔥 feat: narrow client hook lock scope to avoid serializing concurrent requests

4 participants