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

📝 [Proposal]: Revert of Reject Middleware context.Context PRs #3358

Open
3 tasks done
sixcolors opened this issue Mar 18, 2025 · 14 comments
Open
3 tasks done

📝 [Proposal]: Revert of Reject Middleware context.Context PRs #3358

sixcolors opened this issue Mar 18, 2025 · 14 comments

Comments

@sixcolors
Copy link
Member

sixcolors commented Mar 18, 2025

Feature Proposal Description

We should Revert of Reject Middleware context.Context PRs.

Ref: #3339 #3340 #3364 #3365 #3175 #3287

If the goal is to make a service layer framework-agnostic, this approach does not achieve that.

These changes do not remove Fiber-specific dependencies—they add redundant storage without solving the actual issue. Instead of making middleware truly agnostic, they force extra overhead onto Fiber users and violate Fiber’s request-response model.


1. The Service Layer Still Needs Fiber-Specific Code

Even after these changes, the service layer still calls FromContext(), which now checks both fiber.Ctx and context.Context.

This means:

  • The service layer remains aware of Fiber's middleware, contradicting the goal of true framework-agnostic design.
  • Instead of cleanly separating concerns, this just adds another storage layer while keeping Fiber-specific logic.

🔹 A truly agnostic service layer would:
✅ Expect values only from context.Context, without needing Fiber-specific utilities.
Require the caller to store values in context.Context before calling the service layer.

These PRs do not fix the real problem—they just make it more convoluted.


2. Unnecessary Redundancy for Fiber Users

For Fiber users who do not use context.Context, these changes introduce pointless duplication:

  • Now, values are stored twice—once in Locals, once in context.Context.
  • Extra allocations are performed on every request, increasing memory and CPU usage.
  • Zero benefit for users who rely only on Fiber’s Locals.

Fiber’s Locals already provides efficient, request-scoped storage. If Fiber itself were designed around context.Context, this discussion would make sense—but that should be a framework-level decision, not a middleware-by-middleware patchwork.

🔴 The result?
❌ Unnecessary overhead for Fiber users.
❌ A second, redundant storage system that serves no real purpose.


3. This Is a Service Layer Problem, Not a Framework Problem

The root issue isn’t Fiber’s middleware—it’s how the service layer expects to retrieve values.

If a service layer is truly framework-agnostic, it should:
✅ Expect context.Context and rely on the caller to store values there before calling it.
✅ Avoid any framework-specific lookup functions.

Instead, these PRs try to bridge both worlds by keeping Fiber-specific lookups and adding a second storage mechanism, which helps neither case.


Conclusion: These PRs Should Be Reverted or Rejected

These PRs do not solve the problem—they just introduce inefficiency:

🛑 They fail to make the service layer framework-agnostic—Fiber-specific logic is still required.
🛑 They add redundant storage, increasing memory usage and allocations for no reason.
🛑 They break Fiber’s design principles by forcing a second, unnecessary request-scoped store.
🛑 If context.Context is truly needed, this should be addressed at the framework level, not middleware-by-middleware.

If we are serious about context.Context support in Fiber, we should focus on #3344 and consider a proper framework-level implementation, instead of merging piecemeal workarounds that introduce yet another redundant storage mechanism. As @pjebs said "Having too many different types of contexts is confusing".

Checklist:

  • I agree to follow Fiber's Code of Conduct.
  • I have searched for existing issues that describe my proposal before opening this one.
  • I understand that a proposal that does not meet these guidelines may be closed without explanation.
@JIeJaitt
Copy link
Contributor

JIeJaitt commented Mar 19, 2025

@sixcolors I agree with you that these pr's don't exist to solve real problems, but simply to facilitate use and avoid writing a lot of repetitive and redundant code in the third point.

For example, I need to use requestID in the service layer, so if I have a hundred interfaces, I need to take the requestID out of the fiber and put it into the go native context every time in the controller layer.

Can we provide a new middleware that copies all the stored key-value pairs from fiber to go native context if the user wants? This would address the three points you raised.

what do you think?

@pjebs
Copy link
Contributor

pjebs commented Mar 19, 2025

Why would the user want a native go context (eg. context.Background() => context.backgroundCtx) if fiber.Context implemented context.Context?

@sixcolors
Copy link
Member Author

Why would the user want a native go context (eg. context.Background() => context.backgroundCtx) if fiber.Context implemented context.Context?

If i was unclear, I agree with your proposal as a possible solution. I think having multiple structs named context is confusing and unnecessary.

@pjebs
Copy link
Contributor

pjebs commented Mar 19, 2025

My response was to jiejait

@sixcolors
Copy link
Member Author

sixcolors commented Mar 19, 2025

@sixcolors I agree with you that these pr's don't exist to solve real problems, but simply to facilitate use and avoid writing a lot of repetitive and redundant code in the third point.

For example, I need to use requestID in the service layer, so if I have a hundred interfaces, I need to take the requestID out of the fiber and put it into the go native context every time in the controller layer.

Can we provide a new middleware that copies all the stored key-value pairs from fiber to go native context if the user wants? This would address the three points you raised.

what do you think?

@JIeJaitt You can solve this issue easily in Fiber v3 with a custom middleware. Instead of modifying every core framework middleware to copy values into context.Context, you can create your own middleware that does this for you.

Example: Copy Fiber Middleware Values to context.Context

package main

import (
	"context"
	"log"

	"github.com/gofiber/fiber/v3"
	"github.com/gofiber/fiber/v3/middleware/requestid"
)

// In a real application, consider using an un-exported custom type for context keys
// to avoid collisions. However, if the service layer is in a separate package,
// this is not possible. In that case, the responsibility will have to be left
// to the developer to ensure no collisions occur.
const requestIDKey string = "request_id"

// Middleware to copy Fiber Middleware Values into Go context.Context
func WithFiberMiddlewareValuesToContext(c fiber.Ctx) error {
	ctx := c.Context()
	requestID := requestid.FromContext(ctx)
	if requestID != "" {
		ctx = context.WithValue(ctx, requestIDKey, requestID)
	}

	// Add other Fiber Middleware Values as needed
	// ...

	// Update the Fiber context with the new context
	c.SetContext(ctx)

	return c.Next()
}

// Example handler that uses the context
func Handler(c fiber.Ctx) error {
	userCtx := c.Context()
	requestID := ServiceLayer(userCtx)
	return c.SendString("Request ID: " + requestID)
}

// Example service layer function that uses the context.Context
func ServiceLayer(ctx context.Context) string {
	requestID, ok := ctx.Value(requestIDKey).(string)
	if !ok {
		return "Request ID not found"
	}
	return requestID
}

func main() {
	app := fiber.New()

	// Add request ID middleware
	app.Use(requestid.New())

	// Add other middleware as needed
	// app.Use(...)

	// Add custom middleware to copy Fiber Locals into Go context.Context for API group
	app.Use(WithFiberMiddlewareValuesToContext)

	// Example route
	app.Get("/", Handler)

	// Check for errors when starting the server
	if err := app.Listen(":3000"); err != nil {
		log.Fatalf("Failed to start server: %v", err)
	}
}

Why This Works Better?

No need to modify Fiber’s core middleware
No unnecessary overhead for Fiber users who don’t need context.Context
Allows service layers to use context.Context if needed
Keeps Fiber's request-response lifecycle intact

This approach avoids the redundancy of storing values in both Locals and context.Context across multiple middlewares, keeping the implementation simple and efficient.

@JIeJaitt
Copy link
Contributor

Why would the user want a native go context (eg. context.Background() => context.backgroundCtx) if fiber.Context implemented context.Context?

Because the key-value pairs stored in Local are not stored in context.

@JIeJaitt
Copy link
Contributor

@sixcolors I agree with you that these pr's don't exist to solve real problems, but simply to facilitate use and avoid writing a lot of repetitive and redundant code in the third point.
For example, I need to use requestID in the service layer, so if I have a hundred interfaces, I need to take the requestID out of the fiber and put it into the go native context every time in the controller layer.
Can we provide a new middleware that copies all the stored key-value pairs from fiber to go native context if the user wants? This would address the three points you raised.
what do you think?

@JIeJaitt You can solve this issue easily in Fiber v3 with a custom middleware. Instead of modifying every core framework middleware to copy values into context.Context, you can create your own middleware that does this for you.

Example: Copy Fiber Middleware Values to context.Context

package main

import (
"context"
"log"

"github.com/gofiber/fiber/v3"
"github.com/gofiber/fiber/v3/middleware/requestid"
)

// In a real application, consider using an un-exported custom type for context keys
// to avoid collisions. However, if the service layer is in a separate package,
// this is not possible. In that case, the responsibility will have to be left
// to the developer to ensure no collisions occur.
const requestIDKey string = "request_id"

// Middleware to copy Fiber Middleware Values into Go context.Context
func WithFiberMiddlewareValuesToContext(c fiber.Ctx) error {
ctx := c.Context()
requestID := requestid.FromContext(ctx)
if requestID != "" {
ctx = context.WithValue(ctx, requestIDKey, requestID)
}

// Add other Fiber Middleware Values as needed
// ...

// Update the Fiber context with the new context
c.SetContext(ctx)

return c.Next()
}

// Example handler that uses the context
func Handler(c fiber.Ctx) error {
userCtx := c.Context()
requestID := ServiceLayer(userCtx)
return c.SendString("Request ID: " + requestID)
}

// Example service layer function that uses the context.Context
func ServiceLayer(ctx context.Context) string {
requestID, ok := ctx.Value(requestIDKey).(string)
if !ok {
return "Request ID not found"
}
return requestID
}

func main() {
app := fiber.New()

// Add request ID middleware
app.Use(requestid.New())

// Add other middleware as needed
// app.Use(...)

// Add custom middleware to copy Fiber Locals into Go context.Context for API group
app.Use(WithFiberMiddlewareValuesToContext)

// Example route
app.Get("/", Handler)

// Check for errors when starting the server
if err := app.Listen(":3000"); err != nil {
log.Fatalf("Failed to start server: %v", err)
}
}

Why This Works Better?

No need to modify Fiber’s core middlewareNo unnecessary overhead for Fiber users who don’t need context.ContextAllows service layers to use context.Context if neededKeeps Fiber's request-response lifecycle intact

This approach avoids the redundancy of storing values in both Locals and context.Context across multiple middlewares, keeping the implementation simple and efficient.

I agree with this, but I wonder if a whole new middleware is provided, like the adaptor middleware, Converter for net/http handlers to/from Fiber request handlers.

This new middleware could copy all the key-value pairs stored in Local to the native Go context.

@sixcolors
Copy link
Member Author

sixcolors commented Mar 19, 2025

I agree with this, but I wonder if a whole new middleware is provided, like the adaptor middleware, Converter for net/http handlers to/from Fiber request handlers.

This new middleware could copy all the key-value pairs stored in Local to the native Go context.

Yes, copying Locals to a context.Context would be trivial to implement. However, best practice in Go is to use unexported keys when storing values in a context.

Since each Fiber middleware is its own package, users do not have access to the unexported keys needed to retrieve values from Locals. This is why each Fiber middleware provides FromLocal functions—to ensure collision-free access to stored values.

Given this, I still conclude that it's best left to the developer to implement based on their specific needs.

@JIeJaitt
Copy link
Contributor

Yes, copying Locals to a context.Context would be trivial to implement. However, best practice in Go is to use unexported keys when storing values in a context.

Since each Fiber middleware is its own package, users do not have access to the unexported keys needed to retrieve values from Locals. This is why each Fiber middleware provides FromLocal functions—to ensure collision-free access to stored values.

Given this, I still conclude that it's best left to the developer to implement based on their specific needs.

I see. I agree with you on all counts.

@sixcolors
Copy link
Member Author

@JIeJaitt, I really appreciate the effort you’ve put into this. I do see the value in making it easier to work with context.Context, but there are some implementation details and conventions that need to be considered. The main challenge is that each middleware is its own package, and using unexported keys in context.WithValue means users wouldn’t have direct access anyway. That’s why we provide FromLocal functions—to ensure safe access without key collisions.

If this were just about using Locals vs context.Context (not both, redundancy), I wouldn’t be too concerned either way. That said, #3344 will likely make this discussion irrelevant, though we’ll need time to impliment and ensure reasonable API stability—fiber.Ctx is a major part of the framework.

I do think there’s always room for a improvement, and I appreciate the discussion around it!

@JIeJaitt
Copy link
Contributor

@sixcolors I am equally honored to participate in any discussion about fiber, any discussion makes fiber better and better!

@pjebs
Copy link
Contributor

pjebs commented Mar 19, 2025

#3344 (comment)

@pjebs
Copy link
Contributor

pjebs commented Mar 30, 2025

#3382

@sixcolors As an experiment I changed fiber.Ctx to a (bare-bone) context.Context to see what the effects would be.
It doesn't make sense for it to be anything other than bare-bone due to: valyala/fasthttp#965 (comment)

It definitely makes Fiber more straight-forward with zero performance-hit to fiber and its pool.

type context.Context interface {
	Deadline() (deadline time.Time, ok bool)
	Done() <-chan struct{}
	Err() error
	Value(key any)any
}

My suggestion is:

  1. Value refers to fasthttp's uservalues
  2. Deadline, Done and Err all return nil because of Request Cancellation valyala/fasthttp#965 (comment). Maybe in the future they can be properly implemented based on what fasthttp does (unlikely).
  3. Fiber introduces some new functions inspired/based on context pkg:
  • WithDeadline(parent context.Context, d time.Time) (context.Context, CancelFunc)
  • WithTimeout(parent context.Context, timeout time.Duration) (context.Context, CancelFunc)
  • WithCancel(parent context.Context) (ctx context.Context, cancel CancelFunc)

These functions take in a fiber.Context OR some other context.Context and returns a new context directly from the context package. These contexts can be passed to goroutines that outlive the request-response lifecycle. This new context can be loaded with (optionally) copies of the uservalues. They can also be accompanied with a note documenting the performance hit.

Additionally, the setContext and context method is removed because they are no longer necessary.

@sixcolors
Copy link
Member Author

👀 on #3382, looks good so far 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants