aspnet / AspNetWebStack

ASP.NET MVC 5.x, Web API 2.x, and Web Pages 3.x (not ASP.NET Core)
Other
858 stars 354 forks source link

Using IEnumerable<> as return type on controller's GET action method confuses OnActionExecuted event method #406

Closed sim590 closed 1 year ago

sim590 commented 1 year ago

Let the following minimal example at:

https://github.com/sim590/IEnumrableBogueASPNetWebHttpAPI

containing:

How to reproduce

Simply build and start IIS Express through Visual Studio.

Set breakpoints at:

Then, issue a REST request like so:

Invoke-RestMethod -Method GET -Uri "http://localhost:56484/api/Toto"

Observed behaviour

The order of execution of the filter event methods and the controller method are as follows:

  1. Filters/TotoFilter.cs:15
  2. Filters/TotoFilter.cs:19
  3. Controllers/TotoController.cs:16

Expected behaviour

The order of execution of the filter event methods and the controller method should be as follows:

  1. Filters/TotoFilter.cs:15
  2. Controllers/TotoController.cs:16
  3. Filters/TotoFilter.cs:19

Workaround (sort of)

Using the return type List<string> instead of IEnumerable<string> and changing the yield return instruction for the simple return does fix the issue and give the expected behaviour.

CZEMacLeod commented 1 year ago

@sim590 This works as expected, but is perhaps a little unintuitive. When you return IEnumerable (especially with yield return) you are returning a pointer to something that returns the items, not the items. So, when you return List, you are creating a concrete instance of the items. When you return IEnumerable the iterator is not executed until it is serialized, which happens after your filters.

sim590 commented 1 year ago

Yes, I understand what happens, but doesn't it violate the expectation that OnActionExecuted runs after the method? Since the return type is going to be serialized anyway afterwards, shouldn't ASP Net make sure to honor the order of execution of the functions I mention by making sure any lazy evaluated container is forced to be concretely evaluated before any other steps after calling the method?

CZEMacLeod commented 1 year ago

@sim590 As filters may alter the returned content, you might not want to perform all the heavy work of getting the data for the response in the controller. The same happens with returning a Stream or any other 'streaming' return type.

The thing is that your controller method had actually completed and returned the value; e.g. the IEnumerable.

I think the issue is that your 'expectation' (as you put it), is that an iterator method is fully iterated before the method returns, and that is just not the case. In VB you have to decorate the method with the Iterator keyword and it is much more obvious that the method doesn't execute in the normal order. There are a lot of gotchya's here; but it is part of the language.

Just because the enumerator hasn't been iterated over (which is actually where line 16 is called) doesn't mean the execution is out of order. I don't think the framework should be 'opionated' here and force the result to be iterated and stored in memory (which might be an issue). If you want that behaviour, simply ToList() it.

I created some examples in https://github.com/CZEMacLeod/FilterExecutionOrder to show...

I would also point out that it is unlikely that MS will make any changes here as a) it could break a lot of people's code; and would potentially be a performance hit, and b) while WebApi has a compatibility layer for aspnet core, they are unlikely to do any new development on this 'legacy' asp.net 4.x based version.

sim590 commented 1 year ago

I don't think the framework should be 'opionated' here and force the result to be iterated and stored in memory (which might be an issue). If you want that behaviour, simply ToList() it.

I understand. I know that the usage of lazy evaluated containers implies the body of the method is not run until the elements are actually accessed.

I think that it is a reasonable thing to not force it. I just thought that forcing it was also defendable due to the expectation we can have in regard to the behavior of filters. But, I understand both points of view.

I would also point out that it is unlikely that MS will make any changes here as a) it could break a lot of people's code; and would potentially be a performance hit, and b) while WebApi has a compatibility layer for aspnet core, they are unlikely to do any new development on this 'legacy' asp.net 4.x based version.

This is true.

Thanks for your input! I will close this.