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

Add context.Context Support to Additional Middlewares #3212

Open
3 tasks
coderabbitai bot opened this issue Nov 21, 2024 · 7 comments
Open
3 tasks

Add context.Context Support to Additional Middlewares #3212

coderabbitai bot opened this issue Nov 21, 2024 · 7 comments

Comments

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 21, 2024

Based on the discussion in Issue #3175, we need to extend context support to other middlewares for consistency.

Middlewares to update:

  • keyauth
  • csrf
  • session

This will ensure that relevant context can be passed through the context.Context in these middlewares.

Requester: @ReneWerner87

@ReneWerner87
Copy link
Member

code from the request id middleware adjustement
https://github.com/gofiber/fiber/pull/3200/files

@gofiber gofiber deleted a comment from coderabbitai bot Nov 21, 2024
@JIeJaitt
Copy link
Contributor

JIeJaitt commented Nov 21, 2024

@ReneWerner87 We need to carefully discuss which middleware need to add requestID, because from the routing to the control layer to the service layer in this process, I see that most of the Go developers will choose to control layer Fiber's Ctx into UserContext, because Fiber as a web framework its function in the whole architecture in the control layer has ended, the service layer will be conducted specific business processing, in the service layer business more concurrent security context, and fiber context is not concurrent security, and requestId in the microservice link tracking is very important to locate the problem, so here will be proposed to increase the request mw UserContext requestID.

The following is the GPT's answer for reference.
Fiber's context (ctx) is not inherently thread-safe. Each context instance is tied to a specific request and should only be used within that request's handler goroutine. If you need to access ctx data from different goroutines, you should.

  1. Pass specific data you need instead of the entire ctx
  2. Use proper synchronization mechanisms (mutexes, channels) if sharing data
  3. Clone required data from ctx before passing to goroutines

Example of what to avoid.

app.Get(“/”, func(c *fiber.Ctx) error {
    go func() {
        // UNSAFE: accessing ctx from different goroutine
        data := c.Query(“someData”)
    }()
    return nil
})

Safe approach.

app.Get(“/”, func(c *fiber.Ctx) error {
    data := c.Query(“someData”) // Get data first
    go func(safeData string) {
        // SAFE: using copied data
        process(safeData)
    }(data)
    return nil
})

@gaby
Copy link
Member

gaby commented Nov 21, 2024

@JIeJaitt This is for using context.Context, not fasthttp.RequestCtx.

@ReneWerner87 ReneWerner87 changed the title Add Context Support to Additional Middlewares Add Context.Context Support to Additional Middlewares Nov 21, 2024
@ReneWerner87 ReneWerner87 changed the title Add Context.Context Support to Additional Middlewares Add context.Context Support to Additional Middlewares Nov 21, 2024
@vhespanha
Copy link

hey, i'm working on this one, i'll submit the PR tomorrow, tops.

can you assign?

@vhespanha
Copy link

just finished KeyAuth. that was the straightforward one.

for session and CSRF, I'm stuck on some decisions. i see patterns in how data flows between the layers beyond fiber.Ctx, but since this is my first contribution, i could use some guidance. @gaby, @ReneWerner87, or @JIeJaitt, could you help clarify what data should be accessible outside the HTTP layer, so I can make it available through raw context?

for example, there's session metadata, like expiry times, that the service-layer might wanna use to pre-emptively refresh sessions, but it could also be considered internal session management details. WDYT?

@gaby
Copy link
Member

gaby commented Jan 22, 2025

@vhespanha While we discuss this, can you submit a PR for the keyauth middleware?

@gaby gaby moved this from Todo to In Progress in v3 Jan 22, 2025
@vhespanha
Copy link

@gaby submitted a draft one, to make sure i got it right before updating the docs. #3287

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

No branches or pull requests

4 participants