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

To wrap the closer as a nop for request body #7

Merged
merged 1 commit into from
Nov 1, 2018

Conversation

jojohappy
Copy link
Contributor

Signed-off-by: jojohappy [email protected]

变更:
由于 Golang 的 net/http 包在做 client.Do(req) 时默认会调用 req.Body.Close(), 如果我们上传的 Object 是文件时,文件句柄将会被关闭,这样就没有办法在外部程序复用这个文件句柄,同时会有 file already closed 的错误发生。

这里的修改尝试在 io.Reader 外面再封装一个 NopCloser ,默认 Close() 返回 nil。对于文件句柄的关闭操作应当由调用方来处理。

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 92.795% when pulling 884aacb on jojohappy:make_reader_closer_as_a_nop into 7c1d230 on mozillazg:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 92.795% when pulling 884aacb on jojohappy:make_reader_closer_as_a_nop into 7c1d230 on mozillazg:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 92.795% when pulling 884aacb on jojohappy:make_reader_closer_as_a_nop into 7c1d230 on mozillazg:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 92.795% when pulling 884aacb on jojohappy:make_reader_closer_as_a_nop into 7c1d230 on mozillazg:master.

@mozillazg
Copy link
Owner

mozillazg commented Oct 30, 2018

@jojohappy Thanks for your PR!

听起来似乎是可以在传入参数前用 ioutil.NopCloser 包装一下文件 object,不需要在 cos 代码中做这个操作? cos 这边可以在相关 api 文档中加一下这个场景的注意事项。

@jojohappy
Copy link
Contributor Author

@mozillazg 好的,谢谢。不过这样做的初衷是希望对用户透明,让用户不用去关心接口以外的内容。

@mozillazg
Copy link
Owner

@jojohappy req.body 自动 close 是为了防止出现资源泄露,如果直接用 ioutil.NopCloser 包装的话,反而需要用户去关心资源回收的问题。毕竟需要用 ioutil.NopCloser 包装的场景比较少,大部分情况下都是可以安全 close 的,这应该也是 client.Do(req) 中自动调用 close 的考虑。如果我的说法不恰当的话,欢迎一起讨论。

@jojohappy
Copy link
Contributor Author

jojohappy commented Oct 31, 2018

@mozillazg 谢谢你的回复,我同意你的说法。这个 PR 的目的是针对类似 *os.File 为目标的 io.Reader, 由于接口入参是 interface, 所以对于用户来说,他们更清楚的了解需要上传的对象是什么。 对于*os.File, 大家都默认会调用defer file.Close(), 那是不是用户自己来处理对象会比较好呢?当然我会再去找找有没有类似的案例,希望有更好的解决方案。

@mozillazg mozillazg changed the base branch from master to develop October 31, 2018 13:54
@mozillazg
Copy link
Owner

mozillazg commented Oct 31, 2018

@jojohappy 后来又想了一下,修改位置的 body 主要是用于上传文件的场景下,你的修改确实是比较合理的考虑。

@jojohappy
Copy link
Contributor Author

@mozillazg 非常感谢~ 如果后续需要做改进,可以再联系我~

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