WebApiThrottle icon indicating copy to clipboard operation
WebApiThrottle copied to clipboard

Don't use OnSendingHeaders

Open JoachimC opened this issue 8 years ago • 3 comments

As we've decided to terminate the middleware stack and initiate creating the response, we shouldn't use OnSendingHeaders. Particularly to set the status code as middleware further down the stack can't see what response we've decided to return.

I appreciate this might be a breaking change for some people but I think it's the 'right' answer - so hopefully you might consider accepting this change.

JoachimC avatar Mar 10 '17 12:03 JoachimC

To avoid a breaking change you could add an optional param to the middleware constructor and use a if to exclude the OnSendingHeaders section.

stefanprodan avatar Mar 12 '17 21:03 stefanprodan

Hi, thanks for considering my PR.

To me that seems like a shame - to introduce the complexity of a flag to switch a behaviour that I don't believe is correct. To me the current behaviour feels like a bug - and this is a fix to that bug.

So maybe this change could wait for a major release - where breaking changes might be allowable?

JoachimC avatar Mar 13 '17 08:03 JoachimC

I will concede that making this change breaks any code reliant on the last-minute mutation of the response, but...

The currently implemented behavior:

  • does not match that of the ThrottlingHandler which has an immediate mutation
  • mutates the response outside of the middleware's execution scope, preventing any middleware that runs after the fact from making accurate decisions based on the response and any further mutations would be discarded

Based on those two things, to me it appears as though the currently released implementation is invalid and a bug, and that the last-minute mutation behavior just should not be permitted (and can be achieved by attaching this middleware last if it is truly desired).

In short -- I agree with JoachimC.

pandinocoder avatar Dec 04 '19 00:12 pandinocoder