Fix nullref exception and added logging

Emby.Server.Implementations/HttpServer/HttpListenerHost.cs

@@ -518,30 +518,29 @@ namespace Emby.Server.Implementations.HttpServer
                 return;
             }
 
-            var url = context.Request.GetDisplayUrl();
-            _logger.LogInformation("WS {Url}. UserAgent: {UserAgent}", url, context.Request.Headers[HeaderNames.UserAgent].ToString());
-
             try
             {
-                var webSocket = await context.WebSockets.AcceptWebSocketAsync(null).ConfigureAwait(false);
+                _logger.LogInformation("WS Request from {IP}", context.Connection.RemoteIpAddress);
+
+                WebSocket webSocket = await context.WebSockets.AcceptWebSocketAsync().ConfigureAwait(false);
 
                 var connection = new WebSocketConnection(
                     _loggerFactory.CreateLogger<WebSocketConnection>(),
                     webSocket,
-                    context.Connection.RemoteIpAddress)
+                    context.Connection.RemoteIpAddress,
+                    context.Request.Query)
                 {
-                    Url = url,
-                    QueryString = context.Request.Query,
                     OnReceive = ProcessWebSocketMessageReceived
                 };
 
                 WebSocketConnected?.Invoke(this, new GenericEventArgs<IWebSocketConnection>(connection));
 
                 await connection.ProcessAsync().ConfigureAwait(false);
+                _logger.LogInformation("WS closed from {IP}", context.Connection.RemoteIpAddress);
             }
-            catch (WebSocketException ex)
+            catch (Exception ex) // Otherwise ASP.Net will ignore the exception
             {
-                _logger.LogError(ex, "ProcessWebSocketRequest error");
+                _logger.LogError(ex, "WebSocketRequestHandler error");
                 if (!context.Response.HasStarted)
                 {
                     context.Response.StatusCode = 500;

Emby.Server.Implementations/HttpServer/WebSocketConnection.cs

@@ -1,4 +1,6 @@
-using System;
+#nullable enable
+
+using System;
 using System.Buffers;
 using System.IO.Pipelines;
 using System.Net;
@@ -39,47 +41,38 @@ namespace Emby.Server.Implementations.HttpServer
         /// <summary>
         /// Initializes a new instance of the <see cref="WebSocketConnection" /> class.
         /// </summary>
+        /// <param name="logger">The logger.</param>
         /// <param name="socket">The socket.</param>
         /// <param name="remoteEndPoint">The remote end point.</param>
-        /// <param name="logger">The logger.</param>
-        /// <exception cref="ArgumentNullException">socket</exception>
-        public WebSocketConnection(ILogger<WebSocketConnection> logger, WebSocket socket, IPAddress remoteEndPoint)
+        /// <param name="query">The query.</param>
+        public WebSocketConnection(
+            ILogger<WebSocketConnection> logger,
+            WebSocket socket,
+            IPAddress? remoteEndPoint,
+            IQueryCollection query)
         {
-            if (socket == null)
-            {
-                throw new ArgumentNullException(nameof(socket));
-            }
-
-            if (remoteEndPoint != null)
-            {
-                throw new ArgumentNullException(nameof(remoteEndPoint));
-            }
-
-            if (logger == null)
-            {
-                throw new ArgumentNullException(nameof(logger));
-            }
-
+            _logger = logger;
             _socket = socket;
             RemoteEndPoint = remoteEndPoint;
-            _logger = logger;
+            QueryString = query;
 
             _jsonOptions = JsonDefaults.GetOptions();
+            LastActivityDate = DateTime.Now;
         }
 
         /// <inheritdoc />
-        public event EventHandler<EventArgs> Closed;
+        public event EventHandler<EventArgs>? Closed;
 
         /// <summary>
         /// Gets or sets the remote end point.
         /// </summary>
-        public IPAddress RemoteEndPoint { get; private set; }
+        public IPAddress? RemoteEndPoint { get; }
 
         /// <summary>
         /// Gets or sets the receive action.
         /// </summary>
         /// <value>The receive action.</value>
-        public Func<WebSocketMessageInfo, Task> OnReceive { get; set; }
+        public Func<WebSocketMessageInfo, Task>? OnReceive { get; set; }
 
         /// <summary>
         /// Gets the last activity date.
@@ -87,17 +80,11 @@ namespace Emby.Server.Implementations.HttpServer
         /// <value>The last activity date.</value>
         public DateTime LastActivityDate { get; private set; }
 
-        /// <summary>
-        /// Gets or sets the URL.
-        /// </summary>
-        /// <value>The URL.</value>
-        public string Url { get; set; }
-
         /// <summary>
         /// Gets or sets the query string.
         /// </summary>
         /// <value>The query string.</value>
-        public IQueryCollection QueryString { get; set; }
+        public IQueryCollection QueryString { get; }
 
         /// <summary>
         /// Gets the state.
@@ -115,11 +102,6 @@ namespace Emby.Server.Implementations.HttpServer
         /// <exception cref="ArgumentNullException">message</exception>
         public Task SendAsync<T>(WebSocketMessage<T> message, CancellationToken cancellationToken)
         {
-            if (message == null)
-            {
-                throw new ArgumentNullException(nameof(message));
-            }
-
             var json = JsonSerializer.SerializeToUtf8Bytes(message, _jsonOptions);
             return _socket.SendAsync(json, WebSocketMessageType.Text, true, cancellationToken);
         }
@@ -140,7 +122,7 @@ namespace Emby.Server.Implementations.HttpServer
                 int bytesRead = receiveresult.Count;
                 if (bytesRead == 0)
                 {
-                    continue;
+                    break;
                 }
 
                 // Tell the PipeWriter how much was read from the Socket
@@ -154,6 +136,8 @@ namespace Emby.Server.Implementations.HttpServer
                     break;
                 }
 
+                LastActivityDate = DateTime.UtcNow;
+
                 if (receiveresult.EndOfMessage)
                 {
                     await ProcessInternal(pipe.Reader).ConfigureAwait(false);
@@ -162,10 +146,7 @@ namespace Emby.Server.Implementations.HttpServer
 
             if (_socket.State == WebSocketState.Open)
             {
-                await _socket.CloseAsync(
-                    WebSocketCloseStatus.NormalClosure,
-                    string.Empty, // REVIEW: human readable explanation as to why the connection is closed.
-                    cancellationToken).ConfigureAwait(false);
+                _logger.LogWarning("Stopped reading from websocket before it was closed");
             }
 
             Closed?.Invoke(this, EventArgs.Empty);
@@ -175,8 +156,6 @@ namespace Emby.Server.Implementations.HttpServer
 
         private async Task ProcessInternal(PipeReader reader)
         {
-            LastActivityDate = DateTime.UtcNow;
-
             if (OnReceive == null)
             {
                 return;

Emby.Server.Implementations/Session/SessionManager.cs

@@ -1726,6 +1726,7 @@ namespace Emby.Server.Implementations.Session
                 string.Equals(i.Client, client));
         }
 
+        /// <inheritdoc />
         public SessionInfo GetSessionByAuthenticationToken(AuthenticationInfo info, string deviceId, string remoteEndpoint, string appVersion)
         {
             if (info == null)
@@ -1733,7 +1734,7 @@ namespace Emby.Server.Implementations.Session
                 throw new ArgumentNullException(nameof(info));
             }
 
-            var user = info.UserId.Equals(Guid.Empty)
+            var user = info.UserId == Guid.Empty
                 ? null
                 : _userManager.GetUserById(info.UserId);
 

Emby.Server.Implementations/Session/SessionWebSocketListener.cs

@@ -56,7 +56,7 @@ namespace Emby.Server.Implementations.Session
             }
             else
             {
-                _logger.LogWarning("Unable to determine session based on url: {0}", e.Argument.Url);
+                _logger.LogWarning("Unable to determine session based on query string: {0}", e.Argument.QueryString);
             }
         }
 

Emby.Server.Implementations/Session/WebSocketController.cs

@@ -53,11 +53,12 @@ namespace Emby.Server.Implementations.Session
 
         private void OnConnectionClosed(object sender, EventArgs e)
         {
-            _logger.LogDebug("Removing websocket from session {Session}", _session.Id);
             var connection = (IWebSocketConnection)sender;
+            _logger.LogDebug("Removing websocket from session {Session}", _session.Id);
             _sockets.Remove(connection);
-            _sessionManager.CloseIfNeeded(_session);
+            connection.Closed -= OnConnectionClosed;
             connection.Dispose();
+            _sessionManager.CloseIfNeeded(_session);
         }
 
         /// <inheritdoc />

MediaBrowser.Controller/Net/IWebSocketConnection.cs

@@ -1,3 +1,5 @@
+#nullable enable
+
 using System;
 using System.Net;
 using System.Net.WebSockets;
@@ -13,7 +15,7 @@ namespace MediaBrowser.Controller.Net
         /// <summary>
         /// Occurs when [closed].
         /// </summary>
-        event EventHandler<EventArgs> Closed;
+        event EventHandler<EventArgs>? Closed;
 
         /// <summary>
         /// Gets the last activity date.
@@ -21,23 +23,17 @@ namespace MediaBrowser.Controller.Net
         /// <value>The last activity date.</value>
         DateTime LastActivityDate { get; }
 
-        /// <summary>
-        /// Gets or sets the URL.
-        /// </summary>
-        /// <value>The URL.</value>
-        string Url { get; set; }
-
         /// <summary>
         /// Gets or sets the query string.
         /// </summary>
         /// <value>The query string.</value>
-        IQueryCollection QueryString { get; set; }
+        IQueryCollection QueryString { get; }
 
         /// <summary>
         /// Gets or sets the receive action.
         /// </summary>
         /// <value>The receive action.</value>
-        Func<WebSocketMessageInfo, Task> OnReceive { get; set; }
+        Func<WebSocketMessageInfo, Task>? OnReceive { get; set; }
 
         /// <summary>
         /// Gets the state.
@@ -49,7 +45,7 @@ namespace MediaBrowser.Controller.Net
         /// Gets the remote end point.
         /// </summary>
         /// <value>The remote end point.</value>
-        IPAddress RemoteEndPoint { get; }
+        IPAddress? RemoteEndPoint { get; }
 
         /// <summary>
         /// Sends a message asynchronously.