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

fixed work with fiber.Ctx, fiber.UserContext - fixed graceful shutdown and race condition on access to huma.Context outside handler #725

Merged
merged 2 commits into from
Feb 18, 2025

Conversation

excavador
Copy link
Contributor

Fiber and FastHttp has the very strict rules how to work with Context() and UserContext()

If some huma-based application, whatever reason, has references to context.Context outside of Huma handler, then fiber/fasthttp RequestCtx does not work as expected, the most likely you receive crash or race-condition

To prevent race conditions and possible issues with graceful shutdown of the servers, this pull request completely fix the way on how fiber and huma integrated.

It also include golang tests to detect race conditions.

@excavador excavador force-pushed the huma-fiber-context-fixes branch from 01bf44f to 21f7dbf Compare February 12, 2025 13:34
…n and race condition on access to huma.Context outside handler
@excavador excavador force-pushed the huma-fiber-context-fixes branch from 21f7dbf to 3e2188a Compare February 12, 2025 14:49
Copy link

codecov bot commented Feb 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.79%. Comparing base (f9ffb6a) to head (2cd0f27).
Report is 14 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #725   +/-   ##
=======================================
  Coverage   92.79%   92.79%           
=======================================
  Files          22       22           
  Lines        5027     5027           
=======================================
  Hits         4665     4665           
  Misses        312      312           
  Partials       50       50           

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

@excavador excavador force-pushed the huma-fiber-context-fixes branch from 8aba0f9 to 2cd0f27 Compare February 12, 2025 15:37
@excavador
Copy link
Contributor Author

excavador commented Feb 13, 2025

@danielgtaylor I hope this it the final fix of "humafiver" provider.
I am sorry for several iterations, but "zero-allocation" behavior of fasthttp / fiber is not something so simple to prepare properly

In this Pull Request I have the reproduction case to illustrate issues with fasthttp/fiber in case when user-side code ignores recommentations about fiber/fasthttp (and without penalties for that!)
I also added -race flag to golang tests to catch this issue.

This test failed with current "master" implementation, but works perfectly with fixed one

Choose a reason for hiding this comment

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

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

Comments suppressed due to low confidence (2)

adapters/humafiber/humafiber.go:141

  • [nitpick] The name 'contextWrapperValue' is ambiguous. Consider renaming it to 'ContextKeyValuePair' for clarity.
type contextWrapperValue struct {

adapters/humafiber/humafiber.go:146

  • [nitpick] The name 'contextWrapper' is ambiguous. Consider renaming it to 'ContextWithValues' for clarity.
type contextWrapper struct {
@danielgtaylor danielgtaylor merged commit f6ce596 into danielgtaylor:main Feb 18, 2025
5 checks passed
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.

2 participants