ConnectCallback's context.InitialRequestMessage has misleading lifetime (not tied to the Connection)

[MihaZupan]

The ConnectCallback is provided with a SocketsHttpConnectionContext that includes the InitialRequestMessage.
By the time the callback runs, the initial request may have already been completed and returned to the user, or it is being sent by a different connection. That is, a different thread may be accessing the headers while the callback is running.

As a result, any access of InitialRequestMessage's headers is problematic.

Related: #61798, #65379

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

The ConnectCallback is provided with a SocketsHttpConnectionContext that includes the InitialRequestMessage.
By the time the callback runs, the initial request may have already been completed and returned to the user, or it is being sent by a different connection. That is, a different thread may be accessing the headers while the callback is running.

As a result, any access of InitialRequestMessage's headers is problematic.

Related: #61798, #65379

Author:	MihaZupan
Assignees:	-
Labels:	bug, area-System.Net.Http
Milestone:	-

[stephentoub]

I assume this is due to the change in .NET 6 that enables a different connection to service the request if one becomes available before this connection has been established?

[MihaZupan]

Exactly.

[stephentoub]

In general that change would seem to have broken the notion of "InitialRequestMessage", since it's not necessarily. That's still the request that made the system decide to inject another connection, but it's not necessarily the first one to use it, or even use it at all.

[karelz]

Triage:

• Might be good to know how often InitialRequestMessage is used -- perhaps Obsolete it?
• More discussion is needed

[MihaZupan]

Note that this isn't only specific to headers, but anything exposed on HttpRequestMessage.

Headers are the most likely to show up as exceptions, but the ownership of the object is a problem for all the properties.
The user is free to observe/change the properties (Uri / headers / method / Options) after the request is finished, and may not expect those changes to happen while in ConnectCallback.

Worst-case, a user may be reusing the request object entirely (something we don't guard against via HttpMessageInvoker) or using it as a builder for subsequent requests (real scenario).
This could lead to the connect callback establishing a connection to an unrelated endpoint for a wrong connection pool.

[karelz]

Team discussion

Options:

1. Go back to original behavior of associating request with connection and let it block on it
2. Hand over copy of HttpRequestMessage
3. Obsolete it / analyzer + add different properties into the context
4. Make it opt-in
5. Guarantee availability of InitialRequest until first await in the method
6. Add back headers synchronization (although it does not address reusing of requests / modifying it - that would have to be documented)
7. Analyzer to warn against accessing headers in ConnectCallback (in combo with documenting against modifying request as in above)
8. Nullable InitialRequest - as soon as you access it from context, we would "lock it" for the thread

Note: Also PlainTextFilter gets the InitialRequest - that needs to be addressed / documented as well.

We are leaning towards [3].
Perhaps either way do [6].

[karelz]

Continued triage:

• @MihaZupan has solution [6] with little overhead on fast path (in combo with HttpHeaders should not remove invalid values during validating access #67199)
• [3] is needed only for rare cases when users modify the request after processing.
  • As first step we could just document it in the callback
  • We could wait for first users to report it
  • We could ask API review for their opinion
  • Decision: Let's do low-hanging work now -- document. Moving to 7.0. Once that is done, we will move it to Future and wait for reports of problems before we invest more.

[MihaZupan]

The mitigation around headers (concurrent reads) has been merged (#68115) and I've updated the docs to mention that request message reuse is discouraged (dotnet/dotnet-api-docs#8224).

Moving to future.

[karelz]

Team triage:

• We agreed to obsolete the property InitialRequest (with detailed explanation why). Before we do that, we will check top usages on GH (incl. gRPC - @JamesNK) ... it is the least bad solution.

[JamesNK]

Can gRPC keep using it? Or could the request's property bag be provided in the callback? I need some way to pass in state to the callback from the request.

[MihaZupan]

You could suppress the obsoletion warning and use it, but the question is if it behaves how you're expecting it to.

The InitialRequestMessage you see in the callback may not be the first request on that connection. It may not even be sent using the connection the callback is establishing. It may have already been sent and completed before the callback runs, ...

What are you using the properties for in gRPC?

[karelz]

@JamesNK ping on Miha's question above ...

[JamesNK]

The callback is here:
https://github.com/grpc/grpc-dotnet/blob/4bf9312123f8a63caa408ca74d92040e5763beb4/src/Grpc.Net.Client/Balancer/Internal/BalancerHttpHandler.cs#L77-L90

The values are set onto the request here:
https://github.com/grpc/grpc-dotnet/blob/4bf9312123f8a63caa408ca74d92040e5763beb4/src/Grpc.Net.Client/Balancer/Internal/BalancerHttpHandler.cs#L107-L134

Basically, this handler changes the request URI based on the result from load balancing, and also sets some properties on the request's options. The properties in the request options are only used if the callback is run, and they're used to establish the new connection stream.

[antonfirsov]

Passing read-only information to ConnectCallback through InitialRequestMessage.Options seems to be a common practice advanced users do. To me this looks safe and I don't see any alternatives. Are there any cases where it might be problematic? If it is indeed safe and we deprecate the property, shouldn't we expose an alternative API on SocketsHttpConnectionContext to read options?

[MihaZupan]

One problematic case for Options is if you are using a CONNECT proxy tunnel, the request object you see in the callback is something we've created inside the connection pool, not one of your requests (and the options aren't being passed through).
If we had a different API instead of a request, I assume we would account for that and flow it through.

shouldn't we expose an alternative API on SocketsHttpConnectionContext to read options?

Do you mean literally NewApi => InitialRequestMessage.Options, or a different way of supplying those options as well?

Using the request itself still has issues w.r.t. object lifetimes. If the caller wanted to reuse requests in between (including clearing Options), it can't know when it's safe to do so.

[antonfirsov]

One problematic case for Options is if you are using a CONNECT proxy tunnel, the request object you see in the callback is something we've created inside the connection pool, not one of your requests (and the options aren't being passed through).

I think current users of InitialRequestMessage.Options are aware of this, and they do not use proxy tunnels.

Do you mean literally NewApi => InitialRequestMessage.Options

I originally meant this, but we can consider introducing a new ConnectionOptions bag, if we want to solve the proxy connect problem.

If the caller wanted to reuse requests in between

Isn't this something we explicitly discourage?

[antonfirsov]

By the time the callback runs, the initial request may have already been completed and returned to the user, or it is being sent by a different connection. That is, a different thread may be accessing the headers while the callback is running.

The same concern applies to SocketsHttpPlaintextStreamFilterContext.InitialRequestMessage. If we go with obsoletion, we should obsolete both properties.