support response body stream#1414
Conversation
000ef83 to
b32620d
Compare
erikdubbelboer
left a comment
There was a problem hiding this comment.
Looks good at a first glance, I still have to take a deep look at it.
|
i'm also look deep into it, i think the code can be more elegant, do you think use |
I think it's just a naming issue, since request stream was already supported. |
you could change the name such as |
|
@erikdubbelboer do you have any questions ? |
http.go
Outdated
| c.closeOnce.Do(func() { | ||
| c.err = c.closeFunc() | ||
| }) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
For example, sync.Pool has the same object and clientConn is closed multiple times
There was a problem hiding this comment.
I'm not sure I understand. Can you show how Close would be called multiple times?
There was a problem hiding this comment.
I thought it wouldn't be good to return an io.ReadCloser directly, so I added the CloseBodyStream method @erikdubbelboer
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
perIPConn#Close() method is also wrapped in sync.Once ? It also seems reasonable and easier to deal with
There was a problem hiding this comment.
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 <erik@dubbelboer.com>
|
90da197 to
9c0fb41
Compare
9c0fb41 to
5fdfab1
Compare
7d5b4f5 to
3bdc829
Compare
|
Finally had the time to look, looks good. Thanks! |
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 returnio.ReadCloserand lets the user manually release the resource .Finally, I want request to support stream and open it up to developers.