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

Close resp body on plugin call error #20680

Merged
merged 1 commit into from Feb 25, 2016

Conversation

cpuguy83
Copy link
Member

Please provide the following information:

  • What did you do?
    Fix an issue with the plugin system leaking fd's when the plugin has an error
  • How did you do it?
    Make sure the resp body is closed on error
  • How do I see it or verify it?
  • A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@MHBauer
Copy link
Contributor

MHBauer commented Feb 25, 2016

LGTM. Since the whole body has been read, I don't see any reason not to close it right then and there.

Forgetting to close response bodys in error conditions has to be one of the most common errors of all time. It is the same as forgetting to free.

kitty is SO ANGRY.

@vdemeester
Copy link
Member

LGTM 🐯

vdemeester added a commit that referenced this pull request Feb 25, 2016
@vdemeester vdemeester merged commit a13945d into moby:master Feb 25, 2016
@thaJeztah
Copy link
Member

@cpuguy83 candidate for a patch release? (if we have one?)

@cpuguy83 cpuguy83 added this to the 1.10.3 milestone Feb 25, 2016
@cpuguy83
Copy link
Member Author

@thaJeztah makes sense, though not strictly a horrible issue unless there is a misbehaving plugin.

@cpuguy83 cpuguy83 deleted the close_plugin_req_body_on_error branch February 25, 2016 14:51
@cpuguy83 cpuguy83 added the priority/P3 Best effort: those are nice to have / minor issues. label Feb 25, 2016
@thaJeztah thaJeztah removed the priority/P3 Best effort: those are nice to have / minor issues. label Mar 4, 2016
@tiborvass tiborvass mentioned this pull request Mar 7, 2016
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.

None yet

5 participants