Make OwinFeatureCollection mutable

[aspnet-hello]

From @Tratcher on Wednesday, October 7, 2015 9:24:33 AM

OwinFeatureCollection does not currently allow you to replace features, so to use it in the pipeline you have to wrap it in a FeatureCollection. Make it mutable instead.

Copied from original issue: aspnet/HttpAbstractions#431

[aspnet-hello]

From @borgdylan on Sunday, October 11, 2015 5:57:52 AM

As of the time of this comment, wrapping the feature collection makes nowin hang under heavy load. We need the mutable feature collection badly.

[aspnet-hello]

From @Tratcher on Sunday, October 11, 2015 6:03:17 AM

Are you sure the hang is related? The wrapping has always been there, we just stopped doing it by default.

[aspnet-hello]

From @borgdylan on Sunday, October 11, 2015 6:05:37 AM

Without wrapping stuff no longer works. Pressing f5 repeatedly causes a hang. Kestrel does not hang if I do the same thing.

[aspnet-hello]

From @benaadams on Sunday, October 11, 2015 6:54:52 AM

What's heavy load? Is that high requests per second without changing the app? F5 in browser rather than VS? Can you pull some stats from perfmon - like time in GC?

[aspnet-hello]

From @borgdylan on Sunday, October 11, 2015 8:58:33 AM

Its doing a repeated F5 cycle. I am on Linux using mono so perfmon doesn't exist and neither does VS.

[aspnet-hello]

From @borgdylan on Sunday, October 11, 2015 9:08:25 AM

Using the ubuntu system monitor I cannot observe rapid eating of memory with neither kestrel or Nowin.

[aspnet-hello]

From @borgdylan on Sunday, October 11, 2015 9:13:17 AM

Using mono's debug spew for the GC, I can notice a bunch of collections as the app starts up and a few per normal set of requests. However no collections happen during the hang. Could it be a mono bug triggering here?

[aspnet-hello]

From @benaadams on Sunday, October 11, 2015 9:34:14 AM

Mono does seem to jam occasionally during the automated github tests on check in; not sure why, may be similar. Just F5ing in a browser shouldn't cause enough GC memory recycling pressure to hang. I'm guessing F5 will be doing a disconnect/reconnect cycle; rather than using keep-alive on same connection. I assume it worked before the change; though as @Tratcher points out it was just wrapping at in a different function further down the stack. </thinking_aloud>

Can you add a gist or a branch that shows how you are doing the wrapping?

[aspnet-hello]

From @borgdylan on Sunday, October 11, 2015 9:42:53 AM

I am wrapping here https://github.com/borgdylan/aspnet5-repros/blob/master/NowinServerFactory/NowinServerFactory.cs#L26 moving from

var ofc = new OwinFeatureCollection(env);

to

var ofc = new FeatureCollection(new OwinFeatureCollection(env));

[aspnet-hello]

From @borgdylan on Sunday, October 11, 2015 9:43:04 AM

Is it wrong to wrap per request?

[aspnet-hello]

From @benaadams on Sunday, October 11, 2015 10:00:11 AM

Looks like you are doing essentially what it was doing automatically (and often unnecessarily) before; which was per request; I believe.

[aspnet-hello]

From @borgdylan on Sunday, October 11, 2015 10:19:01 AM

In this case, if I remove the wrapping all requests will return a 500. The pipeline is trying to add features and that will fail in read-only collections.

[aspnet-hello]

From @benaadams on Sunday, October 11, 2015 10:19:26 AM

Wrapping is the correct thing to do.

As a complete guess, the jamming will be occurring in the connect/disconnect. Assuming nowin supports keep-alive, could you point wrk at it for load and see how it performs? i.e. it should only make a few connections and reuse them for all requests.

[aspnet-hello]

From @borgdylan on Sunday, October 11, 2015 10:25:22 AM

dylan@ubuntu-server:/var/www/git/wrk$ wrk -t 4 -d 10s -c 4 http://127.0.0.1:8087Running 10s test @ http://127.0.0.1:8087
  4 threads and 4 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     5.76ms   21.36ms 250.62ms   97.99%
    Req/Sec   389.27     62.59   515.00     75.51%
  15262 requests in 10.01s, 58.38MB read
  Socket errors: connect 0, read 1, write 0, timeout 0
  Non-2xx or 3xx responses: 1
Requests/sec:   1524.91
Transfer/sec:      5.83MB

It did not jam at all using wrk as it did with the browser.

[aspnet-hello]

From @borgdylan on Sunday, October 11, 2015 10:28:42 AM

If I go a little overboard:

dylan@ubuntu-server:/var/www/git/wrk$ wrk -t 16 -d 10s -c 32 http://127.0.0.1:8087
Running 10s test @ http://127.0.0.1:8087
  16 threads and 32 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    26.80ms   57.80ms 506.53ms   96.29%
    Req/Sec   124.20     30.23     0.86k    85.82%
  19073 requests in 10.10s, 72.96MB read
Requests/sec:   1888.67
Transfer/sec:      7.22MB
dylan@ubuntu-server:/var/www/git/wrk$ wrk -t 16 -d 10s -c 32 http://127.0.0.1:8087
Running 10s test @ http://127.0.0.1:8087
  16 threads and 32 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     2.69ms    2.00ms  19.31ms   95.27%
    Req/Sec   611.92    184.86     0.95k    59.00%
  18280 requests in 10.02s, 69.92MB read
  Socket errors: connect 0, read 1, write 0, timeout 0
  Non-2xx or 3xx responses: 1
Requests/sec:   1824.25
Transfer/sec:      6.98MB
dylan@ubuntu-server:/var/www/git/wrk$ wrk -t 16 -d 10s -c 32 http://127.0.0.1:8087
Running 10s test @ http://127.0.0.1:8087
  16 threads and 32 connections
^C  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     0.00us    0.00us   0.00us    -nan%
    Req/Sec     0.00      0.00     0.00      -nan%
  0 requests in 2.80s, 0.00B read
Requests/sec:      0.00
Transfer/sec:       0.00B

the server ends up hanging.

[aspnet-hello]

From @borgdylan on Sunday, October 11, 2015 10:32:38 AM

Kestrel fairs better and it does not hang at all. I would use if it supported returning the IP of the caller.

[aspnet-hello]

From @benaadams on Sunday, October 11, 2015 11:39:34 AM

Just trying to understand the code, is it a FeatureCollection wrapped in a OwinFeatureCollection wrapped in a FeatureCollection?

https://github.com/borgdylan/aspnet5-repros/blob/master/NowinServerFactory/NowinServerFactory.cs#L39

[aspnet-hello]

From @borgdylan on Sunday, October 11, 2015 12:29:44 PM

No, its an OwinFeatureCollection inside an FeatureCollection only. I am now running using the bits in aspnetcidev so the removal of the default wrapping is effective on my machine.

In the case of line 39 it is just a FeatureCollection (not wrapping anything) .

[aspnet-hello]

From @borgdylan on Sunday, October 11, 2015 12:36:32 PM

Notice that server features are separate from request features that get made form the environment dictionary handed over by Nowin per request.

[aspnet-hello]

From @benaadams on Sunday, October 11, 2015 1:05:45 PM

Given it some continuous F5s on windows to http://localhost:5001/api/ with the var ofc = new FeatureCollection(new OwinFeatureCollection(env)); change and it seems ok (500s without it); may be mono issue?

How many requests did it need to hang?

[aspnet-hello]

From @borgdylan on Sunday, October 11, 2015 1:10:09 PM

I kept F5 pressed for some time. It hangs just by doing that on a big enough app. The app I linked to on github happens to be big enough. It will hang after a few seconds of F5 being pressed. It will reject all new connections after that.

[aspnet-hello]

From @benaadams on Sunday, October 11, 2015 1:52:37 PM

Did it for about two minutes; chrome (using 50% cpu) would occasionally stop receiving, but if I took finder off button it would complete, then I could hold it down again for longer; so not sure. Thread pool limits on mono? It does/did get upset doing a dnu restore?

As an aside Kestrel could probably do with some way of exposing ip address of caller.

[aspnet-hello]

From @Tratcher on Sunday, October 11, 2015 9:44:10 PM

@borgdylan I recommend taking this discussion elsewhere as we're pretty sure the hang is unrelated to the feature collection.

As for IPs, they're on the list: https://github.com/aspnet/KestrelHttpServer/issues/67

[aspnet-hello]

From @davidfowl on Wednesday, October 14, 2015 4:05:35 AM

@borgdylan Sounds like a nowin bug

[davidfowl]

This seems fine if somebody wants to take a stab at it.