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

OwinFeatureCollection: regression #207

Closed
borgdylan opened this issue Feb 28, 2015 · 18 comments
Closed

OwinFeatureCollection: regression #207

borgdylan opened this issue Feb 28, 2015 · 18 comments

Comments

@borgdylan
Copy link

After some recent changes in the hosting pipeline, the OwinFeatureCollection is no longer handling requests (requests do not even get read by the request pipeline). If I populate a FeatureCollection using an OwinFeatureCollection, the requests gets handled by the request pipeline but the response never surfaces out to the client.

@borgdylan
Copy link
Author

After further investigation, the failures are happening as the setter is throwing a NotSupportedException. Certain features like the one for service providers, send file and request cookies are being set triggering the exception. If one removes the throw statement, nothing is fixed and the same client side behaviour persists.

@Tratcher
Copy link
Member

Tratcher commented Mar 1, 2015

Stack trace?

@borgdylan
Copy link
Author

The method in question is the indexer's setter. Will add a stack trace later.

@davidfowl
Copy link
Member

@borgdylan I'm interested in what changed that started to cause these problems.

@Tratcher
Copy link
Member

Tratcher commented Mar 1, 2015

aspnet/Hosting@435215b

This I bet. The context factory assumed the incoming feature collection was read-only, so it wrapped it in mutable collection. That wrapper was dropped in the last change, so it sounds like you're getting a notsupportedexception trying to call Set on a read-only feature collection.

Sent from my Windows Phone


From: David Fowlermailto:[email protected]
Sent: ‎2/‎28/‎2015 10:37 PM
To: aspnet/HttpAbstractionsmailto:[email protected]
Cc: Chris Rmailto:[email protected]
Subject: Re: [HttpAbstractions] OwinFeatureCollection: regression (#207)

@borgdylan I'm interested in what changed that started to cause these problems.


Reply to this email directly or view it on GitHub:
#207 (comment)

@davidfowl
Copy link
Member

Ah, good catch.

@davidfowl davidfowl added this to the 1.0.0-rc1 milestone Mar 1, 2015
@davidfowl
Copy link
Member

@Praburaj Can you look at this? It's a regression caused by the code changes we made as a result of the API review. Add a test for it as well.

@borgdylan
Copy link
Author

@davidfowl I agree with @Tratcher that this started to occur with IFeatureCollection being needed instead of an env object in the request handling delegate.

@Tratcher Wrapping inside a mutable feature collection solves the exception issue as I have tried it. But the response body still does not get sent back to the client side.

@borgdylan
Copy link
Author

As an aside, Kestrel uses the mutable FeatureCollection directly. With that technique requests work OK. However I do get SignalR errors with respect to invalid delegate instructions.

@Praburaj
Copy link
Contributor

Praburaj commented Mar 2, 2015

@borgdylan @Tratcher @davidfowl - I'll have a look at this today and add a test as well.

@Praburaj
Copy link
Contributor

Praburaj commented Mar 2, 2015

@borgdylan what's the best way to reproduce this issue?

@Praburaj
Copy link
Contributor

Praburaj commented Mar 2, 2015

I tried running a couple of OWIN based entropy samples and they seem to run fine.

@borgdylan
Copy link
Author

i have an updates server factory for nowin. Ill put it on github and you can try with it. It is what I am using.

@borgdylan
Copy link
Author

@Praburaj
Copy link
Contributor

Praburaj commented Mar 2, 2015

@borgdylan these changes already look to be in https://github.com/aspnet/Entropy/tree/dev/samples/Owin.Nowin.HelloWorld sample. This app seems to run fine. Does it require more than this to reproduce this issue?

@borgdylan
Copy link
Author

My code is similar but has a logger for exceptions. Without that logger, the code will fail but the exception will be silently hidden.

@Praburaj
Copy link
Contributor

Praburaj commented Mar 3, 2015

@borgdylan Thanks. Got it.

Sent out a PR. aspnet/Hosting#174

@Praburaj
Copy link
Contributor

Praburaj commented Mar 4, 2015

merged the fix aspnet/Hosting@fde3b0d

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

4 participants