Improve the DefaultShellReleaseManager

[MikeAlhayek]

With this change, we manage the release request inside the ShellScope.

Also, we ensure that the release happens after all the deferred tasks are executed.

Instead of using ShellHost to release the shell, we directly release it using ShellContext.

Follow-up to #15875.

[Piedone]

Let me take a step back here, because I think this starts to become a bit overengineered.

#15875 addresses the use case of drivers releasing the shell context directly. While you didn't state under that PR why this is a problem, I assume what you aim to fix with that is repeated checks for validation errors, potential useless double-calls, and due to the clunkiness of calling IShellHost.ReleaseShellContextAsync() with an injected ShellSettings, some code we could cut down on.

However, to clarify, that PR doesn't fix the following problems, because these weren't problems in the first place:

• The shell being restarted mid-request. IShellHost.ReleaseShellContextAsync() only restarts the shell from the next request, so it's safe to call it anywhere, it won't tear down the shell immediately. (Otherwise, all drivers would've caused various ObjectDisposedExceptions.)
• IShellHost.ReleaseShellContextAsync() being called multiple times. Since our settings drivers check for their editor groups (as every such driver should), their update logic only runs if it's them that's being updated. So, e.g., if you save Localization settings, LocalizationSettingsDisplayDriver will restart the shell, but DefaultSiteSettingsDisplayDriver won't.
• The shell being restarted even if the settings had a validation error. All drivers checked for this. Potentially another driver or something can cause a validation error and thus the shell is restarted still, and that's a valid concern, though I don't see any possibilities of this in the OC code (third-party modules may do it).

I think the changes in this PR defeat the purpose of the first one by re-adding complexity and code, while what we're dealing with is still very limited improvements otherwise.

If the goal is to have a shortcut in the drivers, then that's already achieved. BTW there's no need for a deferred task due to the first point above; I should've pointed out that there's no technical need for any queuing/deferring. That's only necessary to do the release at the end of the scope, without relying on OrchardCore.Settings.Controllers.AdminController to call anything (and thus to make it useful outside of settings too).

[MikeAlhayek]

I think this starts to become a bit overengineered.

How is this over engineering?

Previously it seems awkward to call the _release from the deferred task even when it worked. @sebastienros also mentioned the same thing concern. He also mentioned to use the items of the ShellScope which is what is done here. Also, the request to release the shell is done after all the deferred tasks are completed which to me seams more logical as there could be more deferred tasks to be called.

Lastly, instead of having to use the ShellHost to release the context, or locating the scope as we do for every deferred tasks, we use the ShellContext to issue the release in from the scope directly.

I don't think this is complicating the code. To me this PR places the right parts in the right place.

[Piedone]

What do we want to fix apart from the current implementation being "awkward"?

[MikeAlhayek]

Also like to ensure that the release happens after all the deferred tasks are executed.

[Piedone]

Why does that matter, exactly? The deferred tasks will still execute in the old shell, and the new one will only be built on the next request.

[MikeAlhayek]

Logically it should be the last thing that takes place. But I get your point.

Either way, I believe with the approach in this PR is a bit cleaner.

[Piedone]

I don't really think so. The code is elegant, but IMO unnecessary and adds complexity without addressing a bug/use case.

[hishamco]

I might agree with Zoltan on this, but that doesn't mean the code is not elegant, we might defer it after 2.0 for safety purposes, unless Seb has another opinion