Fixing null reference exception when AzureWebJobsStorage is not set. (#…

…9641)

build/common.props

@@ -5,7 +5,7 @@
     <LangVersion>latest</LangVersion>
     <MajorVersion>4</MajorVersion>
     <MinorVersion>27</MinorVersion>
-    <PatchVersion>5</PatchVersion>
+    <PatchVersion>7</PatchVersion>
     <BuildNumber Condition="'$(BuildNumber)' == '' ">0</BuildNumber>
     <PreviewVersion></PreviewVersion>        
 

src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventNullRepository.cs

@@ -0,0 +1,20 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the MIT License. See License.txt in the project root for license information.
+
+using System;
+using System.Collections.Concurrent;
+using System.Threading;
+using System.Threading.Tasks;
+using Microsoft.Azure.Cosmos.Table;
+using Microsoft.Azure.WebJobs.Host.Executors;
+using Microsoft.Azure.WebJobs.Script.WebHost.Helpers;
+using Microsoft.Extensions.Configuration;
+using Microsoft.Extensions.Logging;
+
+namespace Microsoft.Azure.WebJobs.Script.WebHost.Diagnostics
+{
+    public class DiagnosticEventNullRepository : IDiagnosticEventRepository
+    {
+        public void WriteDiagnosticEvent(DateTime timestamp, string errorCode, LogLevel level, string message, string helpLink, Exception exception) { }
+    }
+}

src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventRepositoryFactory.cs

@@ -2,24 +2,37 @@
 // Licensed under the MIT License. See License.txt in the project root for license information.
 
 using System;
+using Microsoft.Extensions.Configuration;
 using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Logging;
 
 namespace Microsoft.Azure.WebJobs.Script.WebHost.Diagnostics
 {
     public class DiagnosticEventRepositoryFactory : IDiagnosticEventRepositoryFactory
     {
         private readonly IServiceProvider _serviceProvider;
+        private readonly IConfiguration _configuration;
 
-        public DiagnosticEventRepositoryFactory(IServiceProvider serviceProvider)
+        public DiagnosticEventRepositoryFactory(IServiceProvider serviceProvider, IConfiguration configuration)
         {
             _serviceProvider = serviceProvider;
+            _configuration = configuration;
         }
 
         public IDiagnosticEventRepository Create()
         {
             // Using this to break ciruclar dependency for ILoggers. Typically you cannot log errors within the logging pipeline because it creates infinte loop.
             // However in this case that loop is broken because of the filtering in the DiagnosticEventLogger
-            return _serviceProvider.GetRequiredService<IDiagnosticEventRepository>();
+
+            string storageConnectionString = _configuration.GetWebJobsConnectionString(ConnectionStringNames.Storage);
+            if (string.IsNullOrEmpty(storageConnectionString))
+            {
+                return new DiagnosticEventNullRepository();
+            }
+            else
+            {
+                return _serviceProvider.GetRequiredService<IDiagnosticEventRepository>();
+            }
         }
     }
 }

src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventTableStorageRepository.cs

@@ -53,16 +53,16 @@ internal CloudTableClient TableClient
                 if (!_environment.IsPlaceholderModeEnabled() && _tableClient == null)
                 {
                     string storageConnectionString = _configuration.GetWebJobsConnectionString(ConnectionStringNames.Storage);
-                    if (string.IsNullOrEmpty(storageConnectionString))
-                    {
-                        _logger.LogError("Azure Storage connection string is empty or invalid. Unable to write diagnostic events.");
-                    }
-
-                    if (CloudStorageAccount.TryParse(storageConnectionString, out CloudStorageAccount account))
+                    if (!string.IsNullOrEmpty(storageConnectionString)
+                        && CloudStorageAccount.TryParse(storageConnectionString, out CloudStorageAccount account))
                     {
                         var tableClientConfig = new TableClientConfiguration();
                         _tableClient = new CloudTableClient(account.TableStorageUri, account.Credentials, tableClientConfig);
                     }
+                    else
+                    {
+                        _logger.LogError("Azure Storage connection string is empty or invalid. Unable to write diagnostic events.");
+                    }
                 }
 
                 return _tableClient;
@@ -124,10 +124,17 @@ internal virtual async Task FlushLogs(CloudTable table = null)
                 return;
             }
 
-            table = table ?? GetDiagnosticEventsTable();
-
             try
             {
+                table = table ?? GetDiagnosticEventsTable();
+
+                if (table == null)
+                {
+                    _logger.LogError("Unable to get table reference. Aborting write operation");
+                    StopTimer();
+                    return;
+                }
+
                 bool tableCreated = await TableStorageHelpers.CreateIfNotExistsAsync(table, _tableCreationRetries);
                 if (tableCreated)
                 {
@@ -136,11 +143,13 @@ internal virtual async Task FlushLogs(CloudTable table = null)
             }
             catch (Exception ex)
             {
-                _logger.LogError(ex, $"Unable to create table '{table.Name}' after {_tableCreationRetries} retries. Aborting write operation {ex}");
-
+                _logger.LogError(ex, $"Unable to get table reference or create table. Aborting write operation.");
+                return;
+            }
+            finally
+            {
                 // Clearing the memory cache to avoid memory build up.
                 _events.Clear();
-                return;
             }
 
             // Assigning a new empty directory to reset the event count in the new duration window.
@@ -174,9 +183,8 @@ internal async Task ExecuteBatchAsync(ConcurrentDictionary<string, DiagnosticEve
 
         public void WriteDiagnosticEvent(DateTime timestamp, string errorCode, LogLevel level, string message, string helpLink, Exception exception)
         {
-            if (string.IsNullOrEmpty(HostId))
+            if (TableClient == null || string.IsNullOrEmpty(HostId))
             {
-                _logger.LogError("Unable to write diagnostic events. Host id is set to null.");
                 return;
             }
 
@@ -200,6 +208,12 @@ public void WriteDiagnosticEvent(DateTime timestamp, string errorCode, LogLevel
             }
         }
 
+        internal void StopTimer()
+        {
+            _logger.LogInformation("Stopping the flush logs timer");
+            _flushLogsTimer?.Change(Timeout.Infinite, Timeout.Infinite);
+        }
+
         protected virtual void Dispose(bool disposing)
         {
             if (!_disposed)

test/WebJobs.Script.Tests.Integration/Diagnostics/DiagnosticEventTableStorageRepositoryTests.cs

@@ -73,7 +73,7 @@ public void WriteDiagnostic_LogsError_whenHostIdNotSet()
             repository.WriteDiagnosticEvent(DateTime.UtcNow, "eh1", LogLevel.Information, "This is the message", "https://fwlink/", new Exception("exception message"));
 
             var messages = _loggerProvider.GetAllLogMessages();
-            Assert.Equal(messages[0].FormattedMessage, "Unable to write diagnostic events. Host id is set to null.");
+            Assert.Equal(0, repository.Events.Values.Count());
         }
 
         [Fact]
@@ -195,25 +195,35 @@ await TestHelpers.Await(async () =>
         [Fact]
         public async Task FlushLogs_LogsErrorAndClearsEvents_WhenTableCreatingFails()
         {
-            // Clear events by flush logs if table creation attempts fail
+            // Arrange
             IEnvironment testEnvironment = new TestEnvironment();
             testEnvironment.SetEnvironmentVariable(EnvironmentSettingNames.AzureWebsitePlaceholderMode, "0");
 
-            DiagnosticEventTableStorageRepository repository =
-                new DiagnosticEventTableStorageRepository(_configuration, _hostIdProvider, testEnvironment, _logger);
+            var testData = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase)
+            {
+                { "AzureWebJobsStorage", null }
+            };
 
-            var tableClient = repository.TableClient;
-            var cloudTable = tableClient.GetTableReference("aa");
+            var configuration = new ConfigurationBuilder()
+                .AddEnvironmentVariables()
+                .AddInMemoryCollection(testData)
+                .Build();
 
-            repository.WriteDiagnosticEvent(DateTime.UtcNow, "eh1", LogLevel.Information, "This is the message", "https://fwlink/", new Exception("exception message"));
+            DiagnosticEventTableStorageRepository repository =
+                new DiagnosticEventTableStorageRepository(configuration, _hostIdProvider, testEnvironment, _logger);
 
-            Assert.Equal(1, repository.Events.Values.Count());
+            // Act
+            repository.WriteDiagnosticEvent(DateTime.UtcNow, "eh1", LogLevel.Information, "This is the message", "https://fwlink/", new Exception("exception message"));
+            await repository.FlushLogs();
 
-            await repository.FlushLogs(cloudTable);
+            // Assert 
+            var logMessage = _loggerProvider.GetAllLogMessages().SingleOrDefault(m => m.FormattedMessage.Contains("Unable to get table reference"));
+            Assert.NotNull(logMessage);
 
+            var messagePresent = _loggerProvider.GetAllLogMessages().Any(m => m.FormattedMessage.Contains("Azure Storage connection string is empty or invalid. Unable to write diagnostic events."));
+            Assert.True(messagePresent);
+
             Assert.Equal(0, repository.Events.Values.Count());
-            var logMessage = _loggerProvider.GetAllLogMessages()[0];
-            Assert.True(logMessage.FormattedMessage.StartsWith("Unable to create table 'aa'"));
         }
 
         [Fact]