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

support response body stream #1414

Merged

Conversation

Anthony-Dong
Copy link
Contributor

why:
When developing HTTP proxy tools, we encountered the need to forward large packets, and the second is to support streaming forwarding, so we need to modify the sdk.

For security reasons,Response.BodyStream() function return io.ReadCloser and lets the user manually release the resource .

Finally, I want request to support stream and open it up to developers.

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.

Looks good at a first glance, I still have to take a deep look at it.

streaming.go Outdated Show resolved Hide resolved
@byene0923
Copy link
Contributor

i'm also look deep into it, i think the code can be more elegant, do you think use Requeststream in response is a good way?

@Anthony-Dong
Copy link
Contributor Author

i'm also look deep into it, i think the code can be more elegant, do you think use Requeststream in response is a good way?

I think it's just a naming issue, since request stream was already supported.

@byene0923
Copy link
Contributor

i'm also look deep into it, i think the code can be more elegant, do you think use Requeststream in response is a good way?

I think it's just a naming issue, since request stream was already supported.

you could change the name such as bodyStream

@Anthony-Dong
Copy link
Contributor Author

@erikdubbelboer do you have any questions ?

http.go Outdated Show resolved Hide resolved
http.go Outdated Show resolved Hide resolved
http.go Outdated
Comment on lines 328 to 330
c.closeOnce.Do(func() {
c.err = c.closeFunc()
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this sync.Once needed here? If Close can be called multiple times, shouldn't closeFunc then also not be able to be called multiple times?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if SetBodyStream is true, the clientConn does not immediately close. need to close it manually. close clientConn does not need to be executed multiple times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example, sync.Pool has the same object and clientConn is closed multiple times

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand. Can you show how Close would be called multiple times?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it wouldn't be good to return an io.ReadCloser directly, so I added the CloseBodyStream method @erikdubbelboer

Copy link
Collaborator

Choose a reason for hiding this comment

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

I did some investigating into this issue, #1504, and it seems it is cause by Close being called multiple times. So I understand the sync.Once now. But I'm wondering if we should fix the double close instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

perIPConn#Close() method is also wrapped in sync.Once ? It also seems reasonable and easier to deal with

Copy link
Contributor Author

@Anthony-Dong Anthony-Dong Mar 15, 2023

Choose a reason for hiding this comment

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

Whether the client end is used out needs the same processing, I think it depends on whether there is concurrent execution of closeBodyStream. If there is, the same problem will indeed occur, but this is an unreasonable use behavior, not a framework defect. @erikdubbelboer

Co-authored-by: Erik Dubbelboer <[email protected]>
@Anthony-Dong
Copy link
Contributor Author

i'm also look deep into it, i think the code can be more elegant, do you think use Requeststream in response is a good way?

I think it's just a naming issue, since request stream was already supported.

you could change the name such as bodyStream

type responseStream requestStream . might be better

@erikdubbelboer erikdubbelboer merged commit 6b958c2 into valyala:master Apr 5, 2023
@erikdubbelboer
Copy link
Collaborator

Finally had the time to look, looks good. 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.

3 participants