Copy HttpContext properties for long polling transport #1684

davidfowl · 2018-03-22T06:20:54Z

The long polling transport simulates a persistent connection
over multiple http requests. In order to expose common http request
properties, we need to copy them to a fake http context on the first poll
and set that as the HttpContext exposed via the IHttpContextFeature.

- The long polling transport simulates a persistent connection over multiple http requests. In order to expose common http request properties, we need to copy them to a fake http context on the first poll and set that as the HttpContext exposed via the IHttpContextFeature.

- Set the current TraceIdentifier on the fake HttpContext. The trace identifier will reflect accurately in the logs. - Unskip tests

mikaelm12 · 2018-03-22T06:38:17Z

src/Microsoft.AspNetCore.Sockets.Http/HttpConnectionDispatcher.cs

+            features.Set<IHttpConnectionFeature>(connectionFeature);
+
+            // REVIEW: We could strategically look at adding other features but it might be better
+            // if the expose a callback that would allow the user to preserve HttpContext properties.


if we expose a callback...

mikaelm12 · 2018-03-22T06:47:45Z

src/Microsoft.AspNetCore.Sockets.Http/HttpConnectionDispatcher.cs

+            newHttpContext.TraceIdentifier = context.TraceIdentifier;
+            newHttpContext.User = context.User;
+
+            // Making request services function property could be tricky and expensive as it would require


Hmm. Not sure I understand.
Is this something that should be investigated further later on?

Yah, I can remove the comment but I wanted to make it visible in the review.

The comment is fine. Maybe log an issue and link to it

How do DI scopes work with signalr? Why would it be tricky/expensive?

SignalR makes a scope per hub invocation which works great. The RequestServices on the other hand would exist for the lifetime of the connection. We'd then need to dispose the scope when the connection is disposed, that in itself isn't too bad. Where it gets wonky is when somebody tries to get the same instance of a service in middleware and then in a Hub. It'll be different (which is by design and most likely fine). The strange thing about it is that RequestServices would have been replaced.

davidfowl · 2018-03-22T06:49:15Z

test/Microsoft.AspNetCore.SignalR.Tests/HubEndpointTests.cs

@@ -540,7 +540,17 @@ public class HubEndpointTests
                client.Dispose();

                // Ensure the client channel is empty
-                Assert.Null(client.TryRead());
+                var message = client.TryRead();


I had to "fix" this test since it kept failing with the close message thing.

/cc @JamesNK

JamesNK · 2018-03-22T06:55:36Z

src/Microsoft.AspNetCore.Sockets.Http/HttpConnectionDispatcher.cs

+            newHttpContext.RequestServices = EmptyServiceProvider.Instance;
+
+            // REVIEW: This extends the lifetime of anything that got put into HttpContext.Items
+            newHttpContext.Items = context.Items.ToDictionary(p => p.Key, p => p.Value);


Hmmm. To be honest I'm not 100% sure how HttpContext works in websockets land. Is there middleware that would place values in Items?

Also, would this work to copy the dictionary:

newHttpContext.Items = new Dictionary<object, object>(context.Items);

Hmmm. To be honest I'm not 100% sure how HttpContext works in websockets land. Is there middleware that would place values in Items?

The HttpContext is fine in server sent events and websockets, it's in tact because the request doesn't complete until the connection is dead.

Also, would this work to copy the dictionary:

I'll do that not sure why I LINQified this thing 😄

JamesNK · 2018-03-22T06:59:28Z

test/Microsoft.AspNetCore.Sockets.Tests/HttpConnectionDispatcherTests.cs

+                    Assert.Equal(4563, connectionHttpContext.Connection.LocalPort);
+                    Assert.Equal(IPAddress.IPv6Any, connectionHttpContext.Connection.RemoteIpAddress);
+                    Assert.Equal(43456, connectionHttpContext.Connection.RemotePort);
+                    Assert.NotNull(connectionHttpContext.RequestServices);


Also check that doing stuff with the response has no effect?

I'll assert something.

mikaelm12 · 2018-03-22T07:09:03Z

test/Microsoft.AspNetCore.SignalR.Tests/HubEndpointTests.cs

+                {
+                    case CloseMessage close:
+                        break;
+                    case null:


Can you just remove this case since it would be handled by the default case

True. Will do.

mikaelm12 · 2018-03-22T07:09:47Z

test/Microsoft.AspNetCore.Sockets.Tests/HttpConnectionDispatcherTests.cs

@@ -336,6 +338,90 @@ public HttpConnectionDispatcherTests(ITestOutputHelper output) : base(output)
            }
        }

+        [Fact]
+        public async Task HttpContextFeatureForLongpollingWorksBetweenPolls()


I'm sure @moozzyk would love how long this test is 😄

davidfowl · 2018-03-22T07:33:43Z

🆙 📅

JamesNK

One more thing. I don't know all of gotchas of long polling so wait for more approves before merge

JamesNK · 2018-03-22T07:37:00Z

src/Microsoft.AspNetCore.Sockets.Http/HttpConnectionDispatcher.cs

+        private class EmptyServiceProvider : IServiceProvider
+        {
+            public static EmptyServiceProvider Instance { get; } = new EmptyServiceProvider();
+            public object GetService(Type serviceType) => null;


I wonder if this should throw an error explaining why RequestServices doesn't work as expected.

This will at least give a proper exception message when somebody is asking for a required service. That's why I didn't leave it as null. Push come to shove, we can implement something where we make a scoped service provider. I just wanted to see what people think of this first.

What is the error message? I was thinking that calling GetService would throw an error explaining why it isn't supported rather than return null and lead to a hard to debug NRE.

The usage pattern is that the caller does GetService in order to get a null and we have a GetRequiredService that throws if GetService returns null. If anyone is calling GetService today and not expecting null then they already have an NRE

davidfowl · 2018-03-22T08:31:38Z

One more thing. I don't know all of gotchas of long polling so wait for more approves before merge

np

analogrelay

Still a little unsure about this. I don't really like it, but I'm not sure the other hacks are any better...

analogrelay · 2018-03-22T15:31:47Z

src/Microsoft.AspNetCore.Sockets.Http/HttpConnectionDispatcher.cs

+                else
+                {
+                    // Set the request trace identifier to the current http request handling the poll
+                    existing.TraceIdentifier = context.TraceIdentifier;


👍

Any other properties we want to update? Connection info (IP/Port/etc; In case a client changes networks, etc.)?

Of course, this is run in parallel with existing invocations so the data could "shear" (but as long as each set is atomic, it won't break anything).

Extremely unlikely this would matter in real life. The reason I changed the trace identifier is because we log it to tell the user what the previous connection id is.

analogrelay · 2018-03-22T15:49:07Z

src/Microsoft.AspNetCore.Sockets.Http/HttpConnectionDispatcher.cs

+            newHttpContext.RequestServices = EmptyServiceProvider.Instance;
+
+            // REVIEW: This extends the lifetime of anything that got put into HttpContext.Items
+            newHttpContext.Items = new Dictionary<object, object>(context.Items);


This worries me a bit... people use this to cache per-request stuff and we don't want to make those items live longer than the actual request.

This worries me a bit... people use this to cache per-request stuff and we don't want to make those items live longer than the actual request.

Yea, thats why I commented.

BrennanConroy · 2018-03-22T15:54:35Z

src/Microsoft.AspNetCore.Sockets.Http/HttpConnectionDispatcher.cs

-            connection.User = context.User;
-            connection.SetHttpContext(context);
+                // For long polling, the requests come and go but the connection is still alive.
+                // to make the IHttpContextFeature work well, we make a copy of the relevant properties


BrennanConroy · 2018-03-22T16:04:06Z

src/Microsoft.AspNetCore.Sockets.Http/HttpConnectionDispatcher.cs

+                else
+                {
+                    // Set the request trace identifier to the current http request handling the poll
+                    existing.TraceIdentifier = context.TraceIdentifier;


Should we similarly set the existing.User value? Although we use connection.User for the Hub method auth

davidfowl added 2 commits March 21, 2018 23:02

Fixed tests

ee12295

davidfowl requested review from analogrelay, JamesNK, mikaelm12 and BrennanConroy March 22, 2018 06:20

More clean up

ce624a0

- Set the current TraceIdentifier on the fake HttpContext. The trace identifier will reflect accurately in the logs. - Unskip tests

mikaelm12 reviewed Mar 22, 2018

View reviewed changes

Fixed typo

b18924e

mikaelm12 reviewed Mar 22, 2018

View reviewed changes

Ignore close message

45107ec

davidfowl commented Mar 22, 2018

View reviewed changes

JamesNK reviewed Mar 22, 2018

View reviewed changes

mikaelm12 reviewed Mar 22, 2018

View reviewed changes

PR feedback

41076c4

JamesNK approved these changes Mar 22, 2018

View reviewed changes

analogrelay reviewed Mar 22, 2018

View reviewed changes

BrennanConroy reviewed Mar 22, 2018

View reviewed changes

davidfowl added 2 commits March 22, 2018 14:34

Update the user

c7ca72d

Fixed type

ffb0879

davidfowl merged commit f1a3775 into release/2.1 Mar 22, 2018

davidfowl deleted the davidfowl/http-context-longpolling branch March 28, 2018 06:04

cubed-it mentioned this pull request Apr 9, 2020

[Blazor] HttpTransportType.LongPolling vs IHttpContextAccessor dotnet/aspnetcore#20674

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy HttpContext properties for long polling transport #1684

Copy HttpContext properties for long polling transport #1684

davidfowl commented Mar 22, 2018

mikaelm12 Mar 22, 2018

mikaelm12 Mar 22, 2018

davidfowl Mar 22, 2018 •

edited

Loading

mikaelm12 Mar 22, 2018

JamesNK Mar 22, 2018

davidfowl Mar 22, 2018

davidfowl Mar 22, 2018

JamesNK Mar 22, 2018

JamesNK Mar 22, 2018

davidfowl Mar 22, 2018

JamesNK Mar 22, 2018

davidfowl Mar 22, 2018 •

edited

Loading

mikaelm12 Mar 22, 2018

davidfowl Mar 22, 2018

mikaelm12 Mar 22, 2018

moozzyk Mar 23, 2018

davidfowl commented Mar 22, 2018

JamesNK left a comment

JamesNK Mar 22, 2018

davidfowl Mar 22, 2018

JamesNK Mar 22, 2018

davidfowl Mar 22, 2018

davidfowl commented Mar 22, 2018

analogrelay left a comment

analogrelay Mar 22, 2018

davidfowl Mar 22, 2018

analogrelay Mar 22, 2018

davidfowl Mar 22, 2018

BrennanConroy Mar 22, 2018

BrennanConroy Mar 22, 2018

davidfowl Mar 22, 2018

Copy HttpContext properties for long polling transport #1684

Copy HttpContext properties for long polling transport #1684

Conversation

davidfowl commented Mar 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Mar 22, 2018

JamesNK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Mar 22, 2018

analogrelay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl Mar 22, 2018 •

edited

Loading

davidfowl Mar 22, 2018 •

edited

Loading