-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Feature: Add DoRedirects, DoTimeout and DoDeadline to Proxy middleware #2332
Conversation
Signed-off-by: Juan Calderon-Perez <jgcalderonperez@protonmail.com>
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
Signed-off-by: Juan Calderon-Perez <jgcalderonperez@protonmail.com>
Middleware coverage is now at 90.7% up from ~78% |
Do we still need to update the https://github.com/gofiber/docs repo? |
Yeah |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An example would be cool for README
@efectn Example added, PR to docs repo submitted. |
Will review it later today or tomorrow morning |
am not 100% sure yet guess we will also need methods for these 2 things which will come sooner or later, it would be good if we can do this with as little copying and effort as possible https://github.com/valyala/fasthttp/blob/9230a3dd7a778630e568ab4c0f7a5b4340ffa3ab/client.go#L96 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check my comments and try to improve
Sounds good, I will take a look when I get home. |
thx |
Signed-off-by: Juan Calderon-Perez <jgcalderonperez@protonmail.com>
Coverage:
|
@ReneWerner87 This one should be ready for review |
Ok will do the review tomorrow (vacation day) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code looks very good so far
@gaby
Can you add a benchmark and check the master version against the current version for the normal "Do"?
have the assumption that we might have additional allocations by passing the parameters to the inner "doAction" method, which we could then optimize if that is the case
just want to make sure that we always offer a very fast method, without allocations that we could also avoid now already |
@ReneWerner87 To be able to do a benchmark, I had to use the original Test: // go test -v ./... -run=^$ -bench=Benchmark_Proxy_Do_RestoreOriginalURL -benchmem -count=4
func Benchmark_Proxy_Do_RestoreOriginalURL(b *testing.B) {
app := fiber.New()
app.Get("/proxy", func(c *fiber.Ctx) error {
return c.SendString("ok")
})
app.Get("/test", func(c *fiber.Ctx) error {
return Do(c, "/proxy")
})
for n := 0; n < b.N; n++ {
_, _ = app.Test(httptest.NewRequest(fiber.MethodGet, "/test", nil))
}
} Results for
Results after changes from this PR:
|
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Explain the details for making this change. What existing problem does the pull request solve?
Fixes issue reported on Discord
Type of change
Please delete options that are not relevant.
Checklist:
Commit formatting:
Use emojis on commit messages so it provides an easy way of identifying the purpose or intention of a commit. Check out the emoji cheatsheet here: https://gitmoji.carloscuesta.me/