Fix nullability in IOptionsMonitor

[maxkoshevoi]

Quick follow-up to #63767

While annotating Microsoft.Extensions.Logging, it turns out that IOptionsMonitor.OnChange could return null:
https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Logging/src/StaticFilterOptionsMonitor.cs#L16

[ghost]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

Tagging subscribers to this area: @dotnet/area-extensions-options
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Quick follow-up for #63767

While annotating Microsoft.Extensions.Logging, it turns out that IOptionsMonitor.OnChange could return null:
https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Logging/src/StaticFilterOptionsMonitor.cs#L16

Author:	maxkoshevoi
Assignees:	-
Labels:	new-api-needs-documentation, area-Extensions-Options
Milestone:	-

[eerhardt]

LGTM. Thanks!

[deeprobin]

LGTM

[stephentoub]

Is that the only case? Is it likely others have implemented this to return null? Do we document it as possibly returning null?

If this is an isolated case, seems better to fix that one occurrence to not return null; basically it was violating the implicit contract and we're fixing it. It's weird / unexpected for something that returns IDisposable to return null. It could just return a dummy singleton that nops.

[maxkoshevoi]

@stephentoub documentation doesn't say about nullability of the result, but all Extensions.Logging.* assemblies treat it as being nullable.

• There's one other implementation of IOptionsMonitor in this repo (also returns null): https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Logging.Console/src/FormatterOptionsMonitor.cs#L24
• Here're couple in ASP.NET Core (also return null, they are test ones though):
  • https://github.com/dotnet/aspnetcore/blob/main/src/Logging.AzureAppServices/test/OptionsWrapperMonitor.cs#L18
  • https://github.com/dotnet/aspnetcore/blob/main/src/Middleware/HttpLogging/test/OptionsWrapperMonitor.cs#L20

Not sure about other places that implement IOptionsMonitor

[stephentoub]

but all Extensions.Logging.* assemblies treat it as being nullable

Meaning all consumers do null checks on the result?

Sounds then like nullable is the right answer.

Thanks

[eerhardt]

Meaning all consumers do null checks on the result?

Yes. Every place I inspected does null checks before disposing the result. Examples:

runtime/src/libraries/Microsoft.Extensions.Logging/src/LoggerFactory.cs

Line 255 in 9293aab

_changeTokenRegistration?.Dispose();

runtime/src/libraries/Microsoft.Extensions.Logging.Console/src/ConsoleLoggerProvider.cs

Line 189 in 9293aab

_optionsReloadToken?.Dispose();

https://github.com/dotnet/aspnetcore/blob/38bd273d7887dfb408f407d5f6db0d70a8b75e99/src/Logging.AzureAppServices/src/BatchingLoggerProvider.cs#L181

[maxkoshevoi]

Off-topic: @eerhardt could you please tell, how do you do code previews like this?

[eerhardt]

how do you do code previews like this?

It only works for code in the same repo. But the way I do it is using Copy permalink in this context menu:

Then when you paste that link into GH, it shows the code.

runtime/src/libraries/Microsoft.Extensions.Logging.Console/src/ConsoleLoggerProvider.cs

Lines 186 to 191 in 9293aab

	/// <inheritdoc />
	public void Dispose()
	{
	_optionsReloadToken?.Dispose();
	_messageQueue.Dispose();
	}

Your links above are using a branch name, not a commit hash. So in the future, when the code changes, those links will point to different code, whatever happens to be at that line in the future. Using "permalink", or commit hash links, will always point to that code, even if it is deleted in the future.