Dispose the HttpClient when the target closes or reloads - #24
Merged
Merged
Conversation
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The target now overrides FlushAsync: the drain wakes up, sends what is queued and completes the flush once everything enqueued before the call has been sent or given up on. Based on the FlushAsync support in #7. Co-authored-by: Rolf Kristensen <11509660+snakefoot@users.noreply.github.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The tests use the new maxFlushTimeMilliseconds option, so this commit does not compile yet. With only the property added, shutdown takes 45 seconds while the endpoint keeps failing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Closing the target waited for queued logs without a limit, so an endpoint that cannot be reached held the application's shutdown for 45 seconds or more per batch. The new maxFlushTimeMilliseconds option (default 30000, 0 = no limit) bounds that wait, like maxFlushTime in the Java client; when it runs out, a warning goes to NLog's internal log. Based on the bounded CloseTarget wait in #7. Co-authored-by: Rolf Kristensen <11509660+snakefoot@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Stacks this PR on #17. With both, LogFactory.Shutdown() first waits up to NLog's 15 s flush timeout for the drain and then up to maxFlushTimeMilliseconds more on close, so StopsWaitingForDeliveryOnShutdownAfterMaxFlushTime fails (about 15.5 s). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LogFactory.Shutdown() flushes all targets with NLog's own 15 s timeout and then closes them, so with FlushAsync waiting for the drain, closing added maxFlushTimeMilliseconds on top: 15 s + 30 s with the defaults. FlushAsync now completes after at most maxFlushTimeMilliseconds, with a warning in the internal log when it gives up. The time spent waiting on a flush the drain has not delivered yet counts against the close that follows, so a shutdown waits maxFlushTimeMilliseconds in total: with the defaults NLog's flush gives up at 15 s and the close waits the remaining 15 s. A delivered flush does not count, and the drain keeps retrying in the background as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Client has no Dispose() yet, so this commit does not compile. With an empty Dispose(), the disposed client still sends, and a raw http server sees the connections of a reloaded and of a closed target stay open. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Client now implements IDisposable. The target keeps the client it created and disposes it when the drain's Stop() task completes, also when that is after the bounded wait ran out, so a drain still retrying in the background keeps a working client and the process exit is not held up. Each client is disposed once. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PetrHeinz
marked this pull request as ready for review
September 30, 2026 14:54
# Conflicts: # BetterStack.Logs.NLog/BetterStackLogsTarget.cs # tests/BetterStack.Logs.NLog.Tests/BetterStackLogsTargetTests.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #19, which is stacked on #17: the base is
claude/max-flush-time, because this builds on itsstopDrain. Merge it after #17 and #19.Every
InitializeTargetcreates a newClientwith its ownHttpClient, and neitherCloseTargetnor a configuration reload (autoReload) disposes the previous one. Each reload leaks a handler and its pooled connections.ClientimplementsIDisposableand disposes itsHttpClient. This adds to the public API; nothing is removed.stopDraindisposes it when the drain'sStop()task completes, whether or not the bounded wait ran out. It continues the stop task with the disposal instead of disposing right after the wait, so a drain that is still retrying in the background keeps a working client until its loop ends, and the process exit is not held up. Each client is disposed once, also when a closed target is initialized again.Drainis unchanged.The target-level test points two targets (before and after a reload) at a plain TCP server that answers with 202 and keeps the connections open. It checks that the server sees both connections end after the reload and
Shutdown(), with no internal hook in the library.The first commit adds the tests and is expected to be red: it does not compile without
Client.Dispose(). With an emptyDispose()added, the disposed client still sends and both connections stay open. The second commit adds the disposal.🤖 Generated with Claude Code