Skip to content

remove defer #1656

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

Merged
merged 1 commit into from
Nov 20, 2020
Merged

remove defer #1656

merged 1 commit into from
Nov 20, 2020

Conversation

imxyb
Copy link
Contributor

@imxyb imxyb commented Oct 20, 2020

This fd can actually be closed before func return, so i think the defer can remove

@codecov
Copy link

codecov bot commented Oct 20, 2020

Codecov Report

Merging #1656 into master will decrease coverage by 1.50%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1656      +/-   ##
==========================================
- Coverage   85.28%   83.78%   -1.51%     
==========================================
  Files          28       28              
  Lines        2216     1906     -310     
==========================================
- Hits         1890     1597     -293     
+ Misses        212      195      -17     
  Partials      114      114              
Impacted Files Coverage Δ
context.go 86.38% <100.00%> (-3.34%) ⬇️
middleware/method_override.go 80.95% <0.00%> (-3.67%) ⬇️
middleware/redirect.go 85.00% <0.00%> (-3.24%) ⬇️
middleware/proxy.go 63.21% <0.00%> (-3.12%) ⬇️
middleware/basic_auth.go 65.62% <0.00%> (-2.95%) ⬇️
echo.go 83.58% <0.00%> (-2.36%) ⬇️
middleware/util.go 88.88% <0.00%> (-1.74%) ⬇️
response.go 85.18% <0.00%> (-1.66%) ⬇️
middleware/cors.go 77.94% <0.00%> (-1.52%) ⬇️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 151ed6b...4227182. Read the comment docs.

Copy link
Contributor

@pafuent pafuent left a comment

Choose a reason for hiding this comment

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

Maybe in a future version of Echo, the return variables of Context#FormFile could be changed to return also the file descriptor, to avoid the cost of open it from multipart.FileHeader, which is already performed in Go net/http/request stdlib. The only advantage that I can see in the current signature of Context#FormFile is that the user is not responsible for closing the file descriptor, which reduces the cost of a File Descriptor leak vs opening the same file twice.

@lammel lammel merged commit 9676696 into labstack:master Nov 20, 2020
@lammel
Copy link
Contributor

lammel commented Nov 20, 2020

@pafuent I guess we should open a separate issue for changing the Contaxt#FromFile signature, as we will label that for v5 than. Can you open an issue for it, as it was your idea anyway.

Thanks @imxyb , lets get rid of the useless defer now.

@pafuent
Copy link
Contributor

pafuent commented Nov 21, 2020

Sure, the issue is here #1688
I'm working on another PR, but as soon as I finish, I will submit the corresponding PR.

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