HttpListenerContext::AcceptWebSocketAsync should have a nullable annotation for the subProtocol

[euantorano]

Background and Motivation

The method AcceptWebSocketAsync on the HttpListenerContext class currently takes a string subProtocol. When using #nullable enable, the compiler warns about passing null to this parameter, but doing so is totally valid behaviour and indicates no sub-protocol should be used.

Note that passing an empty string (string.Empty) is not valid and causes an exception to be thrown during validation.

The code eventually gets in to HttpWebSocket::ValidateOptions() which has an explicit check for subProtocol begin null, but the parameter is also not marked as nullable there.

Note that currently to ignore the compiler warning you can call the method as follwos:

await ctx.AcceptWebSocketAsync(null!);

Proposed API

UPDATE by @antonfirsov: included all 4 overloads.

public class HttpListenerContext
{
-   public Task<HttpListenerWebSocketContext> AcceptWebSocketAsync(string subProtocol);
+   public Task<HttpListenerWebSocketContext> AcceptWebSocketAsync(string? subProtocol);
-   public Task<HttpListenerWebSocketContext> AcceptWebSocketAsync(string subProtocol, int receiveBufferSize, TimeSpan keepAliveInterval);
+   public Task<HttpListenerWebSocketContext> AcceptWebSocketAsync(string? subProtocol, int receiveBufferSize, TimeSpan keepAliveInterval);
-   public Task<HttpListenerWebSocketContext> AcceptWebSocketAsync(string subProtocol, int receiveBufferSize, TimeSpan keepAliveInterval, ArraySegment<byte> internalBuffer);
+   public Task<HttpListenerWebSocketContext> AcceptWebSocketAsync(string? subProtocol, int receiveBufferSize, TimeSpan keepAliveInterval, ArraySegment<byte> internalBuffer);
-   public Task<HttpListenerWebSocketContext> AcceptWebSocketAsync(string subProtocol, TimeSpan keepAliveInterval);
+    public Task<HttpListenerWebSocketContext> AcceptWebSocketAsync(string? subProtocol, TimeSpan keepAliveInterval);
}

Note: There are several overloads of this method, and it ends up calling these overloads and other methods to validate options - these will also need alterations.

Usage Examples

WebSocketContext webSocketCtx = await httpContext.AcceptWebSocketAsync(null);

Alternative Designs

N/A

Risks

N/A

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

Issue Details

---

Background and Motivation

The method AcceptWebSocketAsync on the HttpListenerContext class currently takes a string subProtocol. When using #nullable enable, the compiler warns about passing null to this parameter, but doing so is totally valid behaviour and indicates no sub-protocol should be used.

Note that passing an empty string (string.Empty) is not valid and causes an exception to be thrown during validation.

The code eventually gets in to HttpWebSocket::ValidateOptions() which has an explicit check for subProtocol begin null, but the parameter is also not marked as nullable there.

Proposed API

-        public Task<HttpListenerWebSocketContext> AcceptWebSocketAsync(string subProtocol)
+       public Task<HttpListenerWebSocketContext> AcceptWebSocketAsync(string? subProtocol)

Note: There are several overloads of this method, and it ends up calling these overloads and other methods to validate options - these will also need alterations.

Usage Examples

WebSocketContext webSocketCtx = await httpContext.AcceptWebSocketAsync(null);

Alternative Designs

N/A

Risks

N/A

Author:	euantorano
Assignees:	-
Labels:	api-suggestion, area-System.Net, untriaged
Milestone:	-

[antonfirsov]

@dotnet/ncl I think this is an overlook we should fix. Note that the repro code in #45934 also uses context.AcceptWebSocketAsync(null!) to overcome this problem.

[karelz]

Triage: This is simple, we should just do it, though it is not pressing. @antonfirsov can you please send quick email to API review, then mark it up-for-grabs and easy? Thanks!

[euantorano]

I made a start at some of the changes for this in my branch, but haven't tested it or anything. It should show the kind of changes needed as far as I can tell, though jumping between projects and solution does make life difficult.

[karelz]

@euantorano looks good to me (I think you are missing ref changes).
If you can help us identify all the public API changes needed, that would be great. I assume it will be just the 4 overloads:

runtime/src/libraries/System.Net.HttpListener/ref/System.Net.HttpListener.cs

Lines 48 to 51 in 8c48ae8

	public System.Threading.Tasks.Task<System.Net.WebSockets.HttpListenerWebSocketContext> AcceptWebSocketAsync(string subProtocol) { throw null; }
	public System.Threading.Tasks.Task<System.Net.WebSockets.HttpListenerWebSocketContext> AcceptWebSocketAsync(string subProtocol, int receiveBufferSize, System.TimeSpan keepAliveInterval) { throw null; }
	public System.Threading.Tasks.Task<System.Net.WebSockets.HttpListenerWebSocketContext> AcceptWebSocketAsync(string subProtocol, int receiveBufferSize, System.TimeSpan keepAliveInterval, System.ArraySegment<byte> internalBuffer) { throw null; }
	public System.Threading.Tasks.Task<System.Net.WebSockets.HttpListenerWebSocketContext> AcceptWebSocketAsync(string subProtocol, System.TimeSpan keepAliveInterval) { throw null; }