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

Merge the various proxy dialer implementations from the fasthttpproxy into a single struct. #1829

Merged
merged 1 commit into from
Aug 24, 2024

Conversation

newacorn
Copy link
Contributor

#1822
In this issue, I noticed that the existing implementation doesn't leverage fasthttp's dialer mechanism, so I created this pull request.
At the same time, provide more comprehensive support for proxy environment variables and allow configuration of the fasthttp dialer.
No tests were committed, and the logic is built on top of the existing fasthttpproxy foundation. I noticed that the original fasthttpproxy also didn't include any test code.
The proxy address is not universal, but I did some local debugging and didn't find any issues.

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.

I like it. I think it makes the old method obsolete, but we can't remove them for backwards compatibility of course. So I think we should rewrite the old methods to use this new Dialer instead. What do you think?

@newacorn
Copy link
Contributor Author

I like it. I think it makes the old method obsolete, but we can't remove them for backwards compatibility of course. So I think we should rewrite the old methods to use this new Dialer instead. What do you think?

You are correct;I agree with your point. I forgot about backward compatibility and deduplication. Next, I will try to rewrite the existing functions.

DialDualStack: true,
}
// proxy information comes from environment
dialer, err := p.GetDialFunc(false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be true if you want the config to come from the env?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thinking was somewhat flawed; I should have combined the three examples together. Thank you.

_ = resp
}

func ExampleDialer_GetDialFunc_three() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the difference between this and the second example? Besides NoProxy: "example.com",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thinking was somewhat flawed; I should have combined the three examples together. Thank you.

Add a multifunctional `Dialer` struct and reimplement the function API

Reimplement the existing function interfaces of the fasthttpproxy package through Dialer. Refactor Dialer.GetDialFunc to ensure that its performance is comparable to the original function interfaces when the proxy does not change with the request address.
@erikdubbelboer erikdubbelboer merged commit 6ed7f6b into valyala:master Aug 24, 2024
15 checks passed
@erikdubbelboer
Copy link
Collaborator

Thanks!

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.

2 participants