Handle an uninitialised appender (#900)

Author: chrisdunelm

apis/Google.Cloud.Logging.V2/Google.Cloud.Logging.Log4Net.Tests/Log4NetTest.cs

@@ -59,7 +59,6 @@ private void LogInfo(IEnumerable<string> messages)
         private class LoggingErrorException : Exception
         {
             public LoggingErrorException(string message) : base(message) { }
-            public LoggingErrorException(string message, Exception e) : base(message, e) { }
         }
 
         private class ThrowingErrorHandler : IErrorHandler
@@ -71,12 +70,12 @@ public void Error(string message)
 
             public void Error(string message, Exception e)
             {
-                throw new LoggingErrorException(message, e);
+                throw e;
             }
 
             public void Error(string message, Exception e, ErrorCode errorCode)
             {
-                throw new LoggingErrorException($"{message} [errorCode={errorCode}]", e);
+                throw e;
             }
         }
 
@@ -178,6 +177,44 @@ private string TimeOfs(int secondsOfs) =>
             (s_time0 + TimeSpan.FromSeconds(secondsOfs))
             .ToString(GoogleStackdriverAppender.s_lostDateTimeFmt);
 
+        [Fact]
+        public async Task UninitialisedBehaviour()
+        {
+            var fakeClient = new Mock<LoggingServiceV2Client>(MockBehavior.Strict);
+            var appender = new GoogleStackdriverAppender(fakeClient.Object, new NoDelayScheduler(), new FakeClock())
+            {
+                ErrorHandler = new ThrowingErrorHandler(),
+                Layout = new PatternLayout { ConversionPattern = "%message" },
+                ProjectId = s_projectId,
+                LogId = s_logId,
+            };
+            // ActivateOptions() has not been called, so appending throws.
+            Assert.Throws<InvalidOperationException>(() => appender.DoAppend(CreateLog(Level.Info, "Message0")));
+            Assert.Throws<InvalidOperationException>(() => appender.Flush(TimeSpan.FromSeconds(1)));
+            await Assert.ThrowsAsync<InvalidOperationException>(() => appender.FlushAsync(TimeSpan.FromSeconds(1)));
+            // But close and dispose can be called fine.
+            // They silently do nothing if the appender is not activated.
+            appender.Close();
+            appender.Dispose();
+        }
+
+        [Fact]
+        public void UninitialisedGced()
+        {
+            var fakeClient = new Mock<LoggingServiceV2Client>(MockBehavior.Strict);
+            var appender = new GoogleStackdriverAppender(fakeClient.Object, new NoDelayScheduler(), new FakeClock())
+            {
+                ErrorHandler = new ThrowingErrorHandler(),
+                Layout = new PatternLayout { ConversionPattern = "%message" },
+                ProjectId = s_projectId,
+                LogId = s_logId,
+            };
+            appender = null;
+            // This should not cause an error on AppenderSkeleton finalization.
+            GC.Collect();
+            GC.WaitForPendingFinalizers();
+        }
+
         [Fact]
         public async Task SingleLogEntry()
         {

apis/Google.Cloud.Logging.V2/Google.Cloud.Logging.Log4Net/GoogleStackdriverAppender.cs

@@ -115,6 +115,7 @@ internal GoogleStackdriverAppender(LoggingServiceV2Client client,
         private ILogQueue _logQ;
         private LogUploader _logUploader;
         private Task<long?> _initIdTask;
+        private bool _isActivated;
         private long _currentId = -1; // Initialised here, in case Flush() is called before any log entries written.
 
         private readonly List<Label> _resourceLabels = new List<Label>();
@@ -192,6 +193,7 @@ public override void ActivateOptions()
                 _client, _scheduler, _clock,
                 _logQ, logsLostWarningEntry, MaxUploadBatchSize,
                 serverErrorBackoffSettings);
+            _isActivated = true;
         }
 
         private void ActivateLogIdAndResource()
@@ -383,13 +385,21 @@ private void Write(IEnumerable<LogEntry> logEntries)
         /// <inheritdoc/>
         protected override void Append(LoggingEvent loggingEvent)
         {
+            if (!_isActivated)
+            {
+                throw new InvalidOperationException($"{nameof(ActivateOptions)}() must be called before using this appender.");
+            }
             var entries = new[] { BuildLogEntry(loggingEvent) };
             Write(entries);
         }
 
         /// <inheritdoc/>
         protected override void Append(LoggingEvent[] loggingEvents)
         {
+            if (!_isActivated)
+            {
+                throw new InvalidOperationException($"{nameof(ActivateOptions)}() must be called before using this appender.");
+            }
             var entries = loggingEvents.Select(x => BuildLogEntry(x)).ToList();
             Write(entries);
         }
@@ -403,6 +413,10 @@ protected override void Append(LoggingEvent[] loggingEvents)
         /// <returns>A task representing whether the flush completed within the timeout.</returns>
         public Task<bool> FlushAsync(TimeSpan timeout, CancellationToken cancellationToken = default(CancellationToken))
         {
+            if (!_isActivated)
+            {
+                throw new InvalidOperationException($"{nameof(ActivateOptions)}() must be called before using this appender.");
+            }
             long untilId;
             lock (_lock)
             {
@@ -418,8 +432,14 @@ protected override void Append(LoggingEvent[] loggingEvents)
         /// <param name="cancellationToken">The token to monitor for cancellation requests.
         /// The default value is <see cref="CancellationToken.None"/>.</param>
         /// <returns>Whether the flush completed within the timeout.</returns>
-        public bool Flush(TimeSpan timeout, CancellationToken cancellationToken = default(CancellationToken)) =>
-            Task.Run(async () => await FlushAsync(timeout, cancellationToken).ConfigureAwait(false)).Result;
+        public bool Flush(TimeSpan timeout, CancellationToken cancellationToken = default(CancellationToken))
+        {
+            if (!_isActivated)
+            {
+                throw new InvalidOperationException($"{nameof(ActivateOptions)}() must be called before using this appender.");
+            }
+            return Task.Run(async () => await FlushAsync(timeout, cancellationToken).ConfigureAwait(false)).Result;
+        }
 
         /// <summary>
         /// Dispose of this appender, by flushing locally buffer entries then closing the appender.
@@ -430,6 +450,11 @@ protected override void Append(LoggingEvent[] loggingEvents)
         /// </remarks>
         public void Dispose()
         {
+            if (!_isActivated)
+            {
+                // Appender not activated, so Dispose is a nop.
+                return;
+            }
             bool flushSucceeded = Flush(TimeSpan.FromSeconds(DisposeTimeoutSeconds));
             if (!flushSucceeded)
             {
@@ -442,6 +467,11 @@ public void Dispose()
         protected override void OnClose()
         {
             base.OnClose();
+            if (!_isActivated)
+            {
+                // Appender not activated, so Close is a nop.
+                return;
+            }
             _logUploader.Close();
         }
     }