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.

Sign up for GitHub

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

Jump to bottom

🐛 [Bug]: v3 ctx.Context() is cancelled upon creation #3186

Closed
3 tasks done
gistart opened this issue Nov 4, 2024 · 6 comments · Fixed by #3193
Closed
3 tasks done

🐛 [Bug]: v3 ctx.Context() is cancelled upon creation #3186

gistart opened this issue Nov 4, 2024 · 6 comments · Fixed by #3193
Assignees
@gaby
Labels
👍 Accepting PR ☢️ Bug 💡 Help wanted

Comments

@gistart
Copy link

gistart commented Nov 4, 2024

Bug Description

Running app normally w/ Listen -- context is fine, so I can use it for my db queries.

fmt.Printf("%+v // %+v\n", c.Context(), c.Context().Err())
// #0000000100000001 - 127.0.0.1:8081<->127.0.0.1:48162 - GET http://0.0.0.0:8081/test // <nil>

Running a test on the same app -- and context is cancelled upon creation

fmt.Printf("%+v // %+v\n", c.Context(), c.Context().Err())
// #0000000100000001 - 0.0.0.0:0<->0.0.0.0:0 - GET http://example.com/test // context canceled

Screenshot from 2024-11-05 00-38-18

How to Reproduce

  1. Create new app
  2. Create any handler
  3. Print context and it's Err()

See code snippet below

Expected Behavior

Context in both prod and test should be initialized as not Done, w/o any errors.

Fiber Version

v3.0.0-beta.3

Code Snippet (optional)

// main.go

package main

import (
	"fmt"

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

func main() {
	app := newApp()
	app.Listen(":8081")
}

func newApp() *fiber.App {
	app := fiber.New()
	app.Get("/test", func(c fiber.Ctx) error {
		fmt.Printf("%+v // %+v\n", c.Context(), c.Context().Err())
		return c.SendString("ok")
	})
	return app
}

// main_test.go

package main

import (
	"net/http/httptest"
	"testing"
)

func TestMain(t *testing.T) {
	app := newApp()

	req := httptest.NewRequest("GET", "/test", nil)
	_, err := app.Test(req, -1)
	if err != nil {
		t.Fatal(err)
	}
}

Checklist:

  • I agree to follow Fiber's Code of Conduct.
  • I have checked for existing issues that describe my problem prior to opening this one.
  • I understand that improperly formatted bug reports may be closed without explanation.
@welcome Welcome
Copy link

welcome bot commented Nov 4, 2024

Thanks for opening your first issue here! 🎉 Be sure to follow the issue template! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

@gaby
Copy link
Member

gaby commented Nov 5, 2024 •
edited
Loading

@gistart I believe you want to use c.UserContext()

The c.Context() returns the Fasthttp context, not the typical Golang context.

@ReneWerner87 We should rename this functions, it's confusing for Golang devs that are used to using Context()

@gistart
Copy link
Author

gistart commented Nov 5, 2024

@gaby oh wow, I didn't realize that I shouldn't use Context()

	// Context returns *fasthttp.RequestCtx that carries a deadline
	// a cancellation signal, and other values across API boundaries.
	Context() *fasthttp.RequestCtx

	// UserContext returns a context implementation that was set by
	// user earlier or returns a non-nil, empty context,if it was not set earlier.
	UserContext() context.Context

I thought it's a good idea to use fiber's context for db queries bcs it might implement all the important logic around user disconnects, request timeouts, graceful shutdowns, etc.

Does UserContext have any of these? If not, could you please share a tip on how to implement it myself?

@ReneWerner87
Copy link
Member

@gistart I believe you want to use c.UserContext()

The c.Context() returns the Fasthttp context, not the typical Golang context.

@ReneWerner87 We should rename this functions, it's confusing for Golang devs that are used to using Context()

which name would be better ?

ctx.ReqCtx()
ctx.RequestCtx()
ctx.CoreCtx()
ctx.FastHttpCtx()

@gaby
Copy link
Member

gaby commented Nov 6, 2024

@gistart I believe you want to use c.UserContext()
The c.Context() returns the Fasthttp context, not the typical Golang context.
@ReneWerner87 We should rename this functions, it's confusing for Golang devs that are used to using Context()

which name would be better ?

ctx.ReqCtx() ctx.RequestCtx() ctx.CoreCtx() ctx.FastHttpCtx()

Probably ctx.RequestCtx() to keep it consistent with fasthttp.

@gaby
Copy link
Member

gaby commented Nov 6, 2024

I will do a PR for this today (renaming).

@gistart I will get back to you on your questions once I read the code a bit more. But I believe that UserContext() is what you want.

With my PR I will rename:

  • Context() to RequestCtx()
  • UserContext() to Context()

@gaby gaby self-assigned this Nov 6, 2024
@gaby gaby mentioned this issue Nov 7, 2024
Merged
4 tasks
JIeJaitt added a commit to JIeJaitt/fiber that referenced this issue Nov 12, 2024
@JIeJaitt
♻️ Refactor: merge issue gofiber#3186
44479ad
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gaby gaby

Labels
👍 Accepting PR ☢️ Bug 💡 Help wanted
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

🐛 fix: Improve naming convention for Context returning functions gofiber/fiber
3 participants
@gistart @gaby @ReneWerner87