OwinFeatureCollection throwing exception in IHttpResponseFeature.OnCompleted and breaks AspNetCore model binding

[damianh]

When running AspNetCore on an owin pipeline, [FromBody] model binding fails because of this exception being thrown:

   at Microsoft.AspNetCore.Owin.OwinFeatureCollection.Microsoft.AspNetCore.Http.Features.IHttpResponseFeature.OnCompleted(Func`2 callback, Object state)
   at Microsoft.AspNetCore.Http.Internal.DefaultHttpResponse.OnCompleted(Func`2 callback, Object state)
   at Microsoft.AspNetCore.Http.HttpResponse.RegisterForDispose(IDisposable disposable)
   at Microsoft.AspNetCore.Http.Internal.BufferingHelper.EnableRewind(HttpRequest request, Int32 bufferThreshold, Nullable`1 bufferLimit)
   at Microsoft.AspNetCore.Mvc.Formatters.JsonInputFormatter.<ReadRequestBodyAsync>d__10.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Mvc.ModelBinding.Binders.BodyModelBinder.<BindModelAsync>d__7.MoveNext()

I copied the OwinFeatureCollection code into my own project and commented out the throw...

... and model binding now works:

[damianh]

Can't believe I'm the first person to hit this 😃

[Tratcher]

Can you provide a concrete code sample? Screen shots make poor repros 😁 Edit: Preferably one that includes your Startup.

[damianh]

Sure, few minutes.

[damianh]

Here you go https://github.com/damianh/AspNetCoreOwinModelBindingRepro/blob/master/src/ConsoleApp1/Program.cs

F5 run should show the stack trace from the ModelState error in the console.

[Tratcher]

Removing the NotSupportException and silently ignoring OnCompleted isn't really an option, it would cause memory leaks. We have to find a way to implement OnCompleted even if means invoking the callbacks a bit early as the OWIN middleware exits. OwinFeatureCollection may need a Complete or Dispose method for the OwinHttpListenerServer sample server to call.

Out of curiosity why are you using Katana as a Core server?

[damianh]

We have a our own app server bootstrapping library. It provides a number of services to hosted applications and it is currently OWIN based. It has been working well for us and we're not ready to replace it. However we want to support teams that use AspNetCore while still supporting the teams that are OWIN based.

In the future, as migrations get further along, we'll likely invert things and host the OWIN apps on an aspnet core based bootstrapper. (If dependency management doesn't prevent us).

[Steve0212a]

What is the status of this? I to am hitting this NotSupportedException(). How am I supposed to work around this?

[Tratcher]

What server are you using and why?

[Steve0212a]

I am not the web developer (on vacation), but I am pretty sure we are using OWin...

I do not know why that is what we are using...

[Tratcher]

OWIN is not a server, it's a spec. What packages are you using?

[Steve0212a]

Attached print screen of references. Sorry again, not the web developer...

[Tratcher]

Interop with Microsoft.Owin.Host.SystemWeb is intentionally not supported. You need to have a long talk with your developer.

[davidfowl]

Right, running ASP.NET Core via OWIN on System.Web isn’t supported. Well take any changes that are reasonable here but going down that path will be fraught with bugs.

[damianh]

Right, running ASP.NET Core via OWIN on System.Web

That's a bit mad alright.

In the future, as migrations get further along, we'll likely invert things and host the OWIN apps on an aspnet core based bootstrapper.

I've since done this so this issue is not a concern to me any more. I guess if we wait long enough the need will go away 😄

[Danielku15]

I today ran into the same issue with a custom IServer implementation I made.

I adapted the sample from here . Like in the example I create a OwinFeatureCollection based on a dictionary.

The keys I supply are:

• owin.RequestProtocol
• owin.RequestPathBase
• owin.RequestScheme
• owin.RequestMethod
• owin.RequestId
• owin.RequestQueryString
• owin.RequestPath
• owin.RequestBody
• server.LocalIpAddress
• server.LocalPort
• server.RemoteIpAddress
• server.RemotePort
• owin.RequestHeaders
• owin.ResponseBody

The values I supply are originally coming from a HttpSys server that lives on another machine so everything should be fine from the content. The custom IServer implementation is a HTTP over WebSocket reverse proxy which I need for architectural reasons.

From the docs it seems that the OnCompleted implementation is quite important. The IServer needs to call the registered callback after the request is complete. It's not yet fully clear to me when exactly this needs to be called (before application.DisposeContext or simply after application.ProcessRequestAsync), but maybe I can find this out by reading the code of the other default implementations.

For me it looks like that OwinFeatureCollection should be an abstract class with OnCompleted to be implemented. Obviously it is incomplete and cannot be directly used, or did I miss something?

[davidfowl]

Yes it seems like it's a problem here especially now since we rely on OnCompleted in a more fundamental way (we use it to dispose of the request scope).

@Danielku15 are you writing an OWIN based server? Or do you just need an IServer implementation?

[Danielku15]

@davidfowl I am developing an IServer implementation which I can use together with ASP.net core (aspnet/mvc) via UseServer<>. I adapted now the FeatureContext class of the HttpSys implementation for my purpose and it is working fine for me. I ended up with a mix of the OwinFeatureCollection and the FeatureContext source.

On topic:

The HttpSys implemementation exposes some methods to the the IServer implementation (MessagePump) which will invoke all registered listeners. Maybe this behavior could be adopted in the base OwinFeatureCollection instead of throwing directly a exception. This way the middlewares ran safely call all methods and the IServer implementation needs to call the related Invoke methods at the correct time.

[davidfowl]

I ended up with a mix of the OwinFeatureCollection and the FeatureContext source.

I'm not sure why as OwinFeatureCollection was a bridge to the past. Unless you're explicitly trying to expose both the OWIN interface and the ASP.NET Core abstraction I don't see why you would need it.

[Danielku15]

In my IServer I receive IDictionary<string, object> environment containing the OWIN compliant keys. The OwinFeatureCollection would be a handy way to execute the received request on the pipeline. Just like in the link I posted earlier.

Here a simplified example of my implementation:

internal class BridgeContract : IServer
{
    private Func<OwinRequestWrapper, Task<OwinResponseWrapper>> _processRequest;

    public IFeatureCollection Features { get; } = new FeatureCollection();
    public BridgeContract() { }

    // called from external
    public async Task<OwinResponseWrapper> ExecuteRequest(OwinRequestWrapper request)
    {
        reutrn await _processRequest(request);
    }

    public Task StartAsync<TContext>(IHttpApplication<TContext> application, CancellationToken cancellationToken)
    {
        _processRequest = async request =>
        {
            IDictionary<string, object> environment = request.ToOwinEnvironment();

            var responseStream = new DisposeNotifyStream();
            environment[OwinKeys.OwinResponseBody] = responseStream;
            environment[OwinKeys.OwinResponseHeaders] = new Dictionary<string, string[]>();

            var owinFeatures = new OwinFeatureCollection(environment);
            var features = new FeatureCollection(owinFeatures);
            var context = application.CreateContext(features);

            try
            {
                await application.ProcessRequestAsync(context);
                // await owinFeatures.OnResponseStart(); 
            }
            catch (Exception ex)
            {
                // await owinFeatures.OnCompleted(); 
                application.DisposeContext(context, ex);
                throw;
            }

            // reset stream for forwarding
            responseStream.Position = 0;
            responseStream.Disposed += () =>
            {
                // await owinFeatures.OnCompleted(); 
                application.DisposeContext(context, null);
            };

            return new OwinResponseWrapper(environment);
        };
        return Task.CompletedTask;
    }

    public Task StopAsync(CancellationToken cancellationToken)
    {
        return Task.CompletedTask;
    }
}

The goal would be to use the OwinFeatureCollection as IFeatureCollection for creating the application context. The ASP.net middlewares (e.g. MVC/WebApi) will call OnCompleted() on the OwinFeatureCollection during the ProcessRequestAsync and currently lead to an exception.

I commented the areas where I now call correctly the OnCompleted and OnStarted callbacks just like the HttpSys implementation.

[davidfowl]

I see, so you already have an OWIN dictionary from somewhere? Where is that dictionary coming from? That's the root of my question.

[Danielku15]

@davidfowl I already described the where my data comes from.

The values I supply are originally coming from a HttpSys server that lives on another machine so everything should be fine from the content. The custom IServer implementation is a HTTP over WebSocket reverse proxy which I need for architectural reasons. (https://github.com/aspnet/Home/issues/3022#issuecomment-415070295)

[davidfowl]

This doesn't really answer my question though. So you have data coming from another server. I don't know that it matters that the server is http.sys. Why is OWIN in the picture? Is it because this server is sending owin specific keys and you're just trying to use them as is?

[Danielku15]

I prepared a Gist implementing a similar architecture than mine (just in-process). When you start it and try to open http://localhost/OWIN/Test you will see in the console that the request will fail with the exception from this issue.

So you have data coming from another server. I don't know that it matters that the server is http.sys.

It matters to me because it highlights that all the keys and values for the owin environment dictionary are valid and correct to be used in the OWIN pipeline. It should eliminate the discussion that my IServer implementation is not correct and is lacking of any OWIN requirements.

But still a dictionary with correct keys will lead to an exception in the OwinFeatureCollection when passed to the IHttpApplication.

Why is OWIN in the picture?

I use the OWIN pipeline to access the raw OWIN environment dictionary for transmission between my internal servers (see gist for details).

[davidfowl]

I wasn't asking about the issue, I was asking about your OWIN usage specifically. It seems like you just made the choice to use OWIN. What we have is now completely unusable and should be fixed. Maybe we should just implement a very basic method for triggering OnCompleted, or make the things that throw virtual so that they can be implemented.

[Danielku15]

I wasn't asking about the issue, I was asking about your OWIN usage specifically. It seems like you just made the choice to use OWIN.

Sorry, I didn't understand that point from the question. Yes I made OWIN my choice here as it seemed to be a nice abstraction layer that is easy to use for my purpose. Wrapping this OWIN environment dictionary for further transmission doesn't require me to invent my own wrapper based on maybe a raw HttpRequestBase and HttpResponseBase object.

Maybe we should just implement a very basic method for triggering OnCompleted, or make the things that throw virtual so that they can be implemented.

That's also what I thought. The other IServer implementations tend to have a public/internal method that will trigger the registered listeners. Employing the same structure in the OwinFeatureCollection would be consistent and easy to use.

[davidfowl]

@Danielku15 feel free to send a pull request with this behavior.

[Danielku15]

Sure, I will try to extend the class and file a pull request on the weekend in my spare time.

[analogrelay]

Closing this as the OWIN interop is not a primary use case and it looks like there are workaround on this thread for all the issues.