Fix nullref exception and added logging

6 files changed, 41 insertions, 65 deletions

diff --git a/Emby.Server.Implementations/HttpServer/HttpListenerHost.cs b/Emby.Server.Implementations/HttpServer/HttpListenerHost.cs
index 4baf96ab5..ebae4d0b1 100644
--- a/Emby.Server.Implementations/HttpServer/HttpListenerHost.cs
+++ b/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;
diff --git a/Emby.Server.Implementations/HttpServer/WebSocketConnection.cs b/Emby.Server.Implementations/HttpServer/WebSocketConnection.cs
index b4f420e5d..88974f9ab 100644
--- a/Emby.Server.Implementations/HttpServer/WebSocketConnection.cs
+++ b/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.
@@ -88,16 +81,10 @@ namespace Emby.Server.Implementations.HttpServer
         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;
diff --git a/Emby.Server.Implementations/Session/SessionManager.cs b/Emby.Server.Implementations/Session/SessionManager.cs
index db00ceeb7..0d5df1dad 100644
--- a/Emby.Server.Implementations/Session/SessionManager.cs
+++ b/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);
 
diff --git a/Emby.Server.Implementations/Session/SessionWebSocketListener.cs b/Emby.Server.Implementations/Session/SessionWebSocketListener.cs
index 13b42698d..d4e4ba1f2 100644
--- a/Emby.Server.Implementations/Session/SessionWebSocketListener.cs
+++ b/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);
             }
         }
 
diff --git a/Emby.Server.Implementations/Session/WebSocketController.cs b/Emby.Server.Implementations/Session/WebSocketController.cs
index c17e67da9..536013c7a 100644
--- a/Emby.Server.Implementations/Session/WebSocketController.cs
+++ b/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 />
diff --git a/MediaBrowser.Controller/Net/IWebSocketConnection.cs b/MediaBrowser.Controller/Net/IWebSocketConnection.cs
index e2a714d5b..d5555884d 100644
--- a/MediaBrowser.Controller/Net/IWebSocketConnection.cs
+++ b/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.
@@ -22,22 +24,16 @@ namespace MediaBrowser.Controller.Net
         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.