Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Stop registering disposable objects in our controller helper methods #4509

Closed
javiercn opened this issue Apr 20, 2016 · 2 comments
Closed

Stop registering disposable objects in our controller helper methods #4509

javiercn opened this issue Apr 20, 2016 · 2 comments
Assignees
Milestone

Comments

@javiercn
Copy link
Member

We have methods on Controller and ControllerBase like
Json(object)`` Ok(object)` that register the result object for disposal automatically by doing the following:

var disposableValue = value as IDisposable;
if (disposableValue != null)
{
    Response.RegisterForDispose(disposableValue);
}

This is problematic because the user has no way of knowing that we are registering something for disposal, so chances are the user registers it for disposal anyways and this behavior might be lost if the user decides to override the method and call base (for example when calling Ok(object)).

As an alternative, it might be possible to provide an ActionFilter that does it for you. That way you can still have this behavior by default if you choose to do so by placing the filter in the list of global filters or putting it on the controllers/actions where you have disposable content but the intention is clear.

To make sure this bug is clear, the goal of this bug is to remove

var disposableValue = value as IDisposable;
if (disposableValue != null)
{
    Response.RegisterForDispose(disposableValue);
}

from the helper methods on our controller and make the user be responsible for registering the objects they return that are disposables.

@Eilon Eilon added this to the 1.0.0-rc2 milestone Apr 21, 2016
@Kukkimonsuta
Copy link

Note that this change will make these methods inconsistent with File method with Stream parameter as FileStreamResult will dispose the stream anyway. It would be nice to have this behavior documented in xml comments. Alternatively if the FileStreamResult gets modified not to dispose, it still should to be documented as it's different behavior from previous mvc versions.

@javiercn
Copy link
Member Author

I was aware of that, but thanks for bringing it up. The line here is do not cast random objects to IDisposable in order to register them. For action results that clearly use resources, like FileStreamResult, let the action result be responsible for disposing it. (That I agree deserves a comment on the class xml docs)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants