-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
client.go: getClientName() may be removed #1458
Comments
Correct. The code was copied from HostClient. I think it did something in the past that isn't relevant anymore. Can you make a pull request? |
stokito
added a commit
to stokito/fasthttp
that referenced
this issue
Jan 1, 2023
The getClientName() checks if !NoDefaultUserAgentHeader then returns the Client.Name field. But it also saves it to atomic field clientName. This is not needed and logic can be simplified. Previously the clientName vas a byte slice that was copied from c.Name and cached. See 02e0722 Fix valyala#1458
Ideally we can make an interface for a client with Name() and NoDefaultUserAgentHeader() and make a generic function but not sure if the small change worth it |
stokito
added a commit
to stokito/fasthttp
that referenced
this issue
Jan 3, 2023
The serverName atomic.Value field is used as a cache. This is not needed and logic can be simplified. See related valyala#1458
erikdubbelboer
pushed a commit
that referenced
this issue
Jan 6, 2023
The serverName atomic.Value field is used as a cache. This is not needed and logic can be simplified. See related #1458
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
When client performs a request it sets a UA if it wasn't set:
The getClientName() checks if !NoDefaultUserAgentHeader then returns the Client.Name field.
But it also saves it to atomic field
clientName
:From what I see the clientName is basically just not needed. It never changes.
We can safely simplify to something like:
Is there any reason to have that atomic or I get something wrong?
@kiyonlin you was a last who changed that code, please participate
02e0722
The text was updated successfully, but these errors were encountered: