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

Abstracts the RoundTripper interface and provides a default implement #1602

Merged
merged 3 commits into from
Aug 10, 2023

Conversation

timandy
Copy link
Contributor

@timandy timandy commented Aug 3, 2023

close #1601

@timandy timandy force-pushed the master branch 2 times, most recently from 8f48ae4 to 5b455a5 Compare August 3, 2023 02:47
@timandy
Copy link
Contributor Author

timandy commented Aug 3, 2023

@erikdubbelboer PTAL, We want to use this feature, Thanks a lot!

@timandy
Copy link
Contributor Author

timandy commented Aug 4, 2023

code style problem be resolved

client.go Outdated Show resolved Hide resolved
type TransportFunc func(*Request, *Response) error
// RoundTripper wraps every request/response.
type RoundTripper interface {
RoundTrip(hc *HostClient, req *Request, resp *Response) (retry bool, err error)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It feels weird to have *HostClient in there, why is that needed? We're going to have people who try to implement the RoundTripper by modifying something and then trying to call hc.Do for example. Which will result in an infinite loop.

Copy link
Contributor Author

@timandy timandy Aug 8, 2023

Choose a reason for hiding this comment

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

It feels weird to have *HostClient in there, why is that needed?

RoundTripe relies on hc's fields and methods to create and release conns. If move these settings into the Transport struct may cause forward compatibility issues.

We're going to have people who try to implement the RoundTripper by modifying something and then trying to call hc.Do for example. Which will result in an infinite loop.

The TransportWrapper testcase shows how to implement a coustom RoundTripper like go net sdk.

client_test.go Outdated

func TestClientTransportEx(t *testing.T) {
var reqLog string
var respLog string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not create TransportEx here, change it to func (t *TransportEx) RoundTrip and set hc.Transport = &tp so you don't have to use string pointers?

Copy link
Contributor Author

@timandy timandy Aug 8, 2023

Choose a reason for hiding this comment

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

I have rewrite the testacase. Please check again. Thanks!
Using interfaces is more extensible than using function, and it is easier to add methods to RoundTripper in the future.

client.go Outdated Show resolved Hide resolved
Co-authored-by: Erik Dubbelboer <erik@dubbelboer.com>
@timandy timandy requested a review from erikdubbelboer August 9, 2023 09:14
@erikdubbelboer erikdubbelboer merged commit 54fdc7a into valyala:master Aug 10, 2023
@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.

Support default Transport implemention for HostClient
2 participants