Determine next log file number from directory on first run

[dustinsoftware]

Determine next log file number from directory on first run

• You've read the Contributor Guide and Code of Conduct.
• You've included unit or integration tests for your change, where applicable.
• You've included inline docs for your change, where applicable.
• There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Summary of the changes (Less than 80 chars)

Description

Fixes #41326

[wtgodbe]

Could you explain how this is different from the current logic? We already attempt to pick up where we left off on restart:

aspnetcore/src/Middleware/HttpLogging/src/FileLoggerProcessor.cs

Lines 151 to 171 in bcdb13d

	private async Task WriteMessagesAsync(List<string> messages, CancellationToken cancellationToken)
	{
	// Files are written up to _maxFileSize before rolling to a new file
	DateTime today = SystemDateTime.Now;
	var fullName = GetFullName(today);
	// Don't write to an incomplete file left around by a previous FileLoggerProcessor
	if (_firstFile)
	{
	while (File.Exists(fullName))
	{
	_fileNumber++;
	if (_fileNumber >= W3CLoggerOptions.MaxFileCount)
	{
	_maxFilesReached = true;
	// Return early if log directory is already full
	Log.MaxFilesReached(_logger);
	return;
	}
	fullName = GetFullName(today);
	}
	}

There is a bug on line 162, though, which should be if (_fileNumber >= W3CLoggerOptions.MaxFileCount - 1)

[dustinsoftware]

I added a test in FileLoggerProcessorTests.cs to clarify what this logic is intended to address. If you stick a breakpoint on this line, the fileCounter is now starting at 4 when the processor is constructed again. Previously it would start back over at 0.

https://github.com/dotnet/aspnetcore/pull/41375/files#diff-eb77a779f77a9a29a358efd0b9aedeb6941383ce8b25a701da4bf9e868c7b433R395

There is a bug on line 162, though, which should be if (_fileNumber >= W3CLoggerOptions.MaxFileCount - 1)

I'll fix that and add a test.

[wtgodbe]

I see, seems like a good behavioral change. Thanks for the catch & fix!

[dustinsoftware]

Tests are updated, let me know if there's anything missing.

With this PR, the assignment to _fileNumber inside the _firstFile block should always be the number of the (not yet written) new log file, and if the value is 10000 we know the folder has been filled up, so I think as-is the check is correct. There is now a test asserting that is the case for both the app-running and app-restarted cases.

[dustinsoftware]

Would it be possible to backport this fix into net6 since that is LTS?

[wtgodbe]

Would it be possible to backport this fix into net6 since that is LTS?

Generally we hesitate to take behavioral changes back to servicing since somebody might be relying on the old behavior, but in this case that seems unlikely. I'll open a backport of this against the release/6.0 branch & see if we can get it through the review process.

[wtgodbe]

/backport to release/6.0

[github-actions]

Started backporting to release/6.0: https://github.com/dotnet/aspnetcore/actions/runs/2259120895

[github-actions]

@wtgodbe backporting to release/6.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Determine next log file number from directory on first run
Using index info to reconstruct a base tree...
M	src/Middleware/HttpLogging/src/FileLoggerProcessor.cs
M	src/Middleware/HttpLogging/test/FileLoggerProcessorTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Middleware/HttpLogging/test/FileLoggerProcessorTests.cs
CONFLICT (content): Merge conflict in src/Middleware/HttpLogging/test/FileLoggerProcessorTests.cs
Auto-merging src/Middleware/HttpLogging/src/FileLoggerProcessor.cs
CONFLICT (content): Merge conflict in src/Middleware/HttpLogging/src/FileLoggerProcessor.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Determine next log file number from directory on first run
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

[wtgodbe]

Ah, right, file-scoped-namespaces. @dustinsoftware could you open a backport of this against the release/6.0 branch, and I'll handle getting it through shiproom review? Branches for servicing are opening tomorrow thru next Monday, so if we can get it in by then it'll be in 6.0 for the June release.

[BrennanConroy]

Ah, right, file-scoped-namespaces

Thought that was fixed with #41087?

[wtgodbe]

Thought that was fixed with #41087?

I thought that only affected git blame

[dustinsoftware]

@dustinsoftware could you open a backport of this against the release/6.0 branch, and I'll handle getting it through shiproom review

sure :)

[ghost]

Hi @dustinsoftware. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.