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

Request timeout settings for the same domain name are reused #1558

Merged
merged 3 commits into from
May 14, 2023

Conversation

byte0o
Copy link
Contributor

@byte0o byte0o commented May 9, 2023

fix client http SetReadDeadline/SetWriteDeadline Deadline is reused

fix client http SetReadDeadline/SetWriteDeadline Deadline is reused
@byte0o byte0o changed the title Update client.go Request timeout settings for the same domain name are reused May 10, 2023
Copy link
Collaborator

@erikdubbelboer erikdubbelboer left a comment

Choose a reason for hiding this comment

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

Great find!

It seems like you have to run gofmt to format the code properly. You're using spaces now but Go code is always indented with tabs.

The comments also aren't needed.

And it seems like some tests are failing now because they don't expect these functions to be called. Can you look into that?

client.go Outdated Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
@erikdubbelboer erikdubbelboer merged commit 9bc8e48 into valyala:master May 14, 2023
@erikdubbelboer
Copy link
Collaborator

Thanks!

@ReneWerner87
Copy link
Contributor

i have tested fiber with the master version and found this error

image
image

image

do i have to adjust anything else for us or is it a real error that has not been noticed ?
@byte0o @erikdubbelboer

https://github.com/gofiber/fiber/blob/9effdf829a79df49c24318bf753f8332d6e75375/client_test.go#L608-L642

@erikdubbelboer
Copy link
Collaborator

@ReneWerner87 as you can see from the stacktrace the errors is in (*readErrorConn).SetWriteDeadline which doesn't exist in https://github.com/gofiber/fiber/blob/master/client_test.go#L584 so I'm not sure which code you are running? But the issue isn't in fasthttp.

@ReneWerner87
Copy link
Contributor

Ok I see, so we need to add these 2 methods to our test connection as well

Thank you for the clarification
(have a nice weekend)

ReneWerner87 added a commit to gofiber/fiber that referenced this pull request Jun 20, 2023
"Request timeout settings for the same domain name are reused #1558" valyala/fasthttp#1558
ReneWerner87 added a commit to gofiber/fiber that referenced this pull request Jun 20, 2023
* Bump github.com/valyala/fasthttp from 1.47.0 to 1.48.0

Bumps [github.com/valyala/fasthttp](https://github.com/valyala/fasthttp) from 1.47.0 to 1.48.0.
- [Release notes](https://github.com/valyala/fasthttp/releases)
- [Commits](valyala/fasthttp@v1.47.0...v1.48.0)

---
updated-dependencies:
- dependency-name: github.com/valyala/fasthttp
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

* repair test setup for this change
"Request timeout settings for the same domain name are reused #1558" valyala/fasthttp#1558

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: René Werner <[email protected]>
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.

3 participants