OSOE-413: Lock to prevent error.

[sarahelsaig]

OSOE-413

An unhandled exception has occurred while executing the request. System.InvalidOperationException: Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.

[Piedone]

This will serialize this part of all requests using the theme in the app. Can't this happen once during app start, for example?

[sarahelsaig]

I was unsure so I did a little experiment:

The breakpoint has tripped on every page load, which tells me that the IOptions<ResourceManagementOptions> is populated every request. I have no idea how else I could prevent this, since i don't have access to the ResourceManagementOptionsConfiguration that adds the ˙bootstrap stylesheet.

I don't think this is necessarily a bottleneck, unless you have a lot of extremely coincident page loads .

[Piedone]

I don't understand how this can cause a concurrency issue, then. That means that this code should only run once per request, and other requests don't bother each other.

If it's somehow a concurrency issue within a single request, then this should rather lock on not a static object but rather something that's unique to a request and available during the whole request.

[sarahelsaig]

It's impossible to be a concurrency issue within the request, because this all happens synchronously.

I think it's because IOptions<T> from services.Configure<T>() is a singleton:

Different requests could totally bother each others' singletons, because requests live in independent scopes, but they share their singletons.

[Piedone]

If it's a singleton, then it's really strange that it's populated on every request. Are you sure?

Taking a step back, why do we need this removal at all? Seems like a hackish solution. We could just name our script resource Site as well like the stylesheet, and include that instead of bootstrap.

[sarahelsaig]

If it's a singleton, then it's really strange that it's populated on every request. Are you sure?

Like I said, I've experimentally verified that it does. We could probably reverse engineer how it gets populated, but that doesn't help us in the short-term.

Taking a step back, why do we need this removal at all?

We use the bootstrap coming from npm and imported to the site stylesheet via the base theme's scss. We don't want to import a duplicate, sometimes older version via the bootstrap resource. But the admin theme (and probably certain built-in widgets) do just that.

Seems like a hackish solution.

Yes, because the resource manager was never designed for removing resources registered in a different module, so we have to hack it.

We could just name our script resource Site as well like the stylesheet, and include that instead of bootstrap.

We already do that, but this standalone "boostrap" resource is needlessly referenced by others. Also you can't overwrite it, because the resources are grouped by module so even if you register an empty resource for "bootstrap" it will be in the Lombiq.BaseTheme's resource dictionary, leaving the one in the admin theme's resource dictionary intact and rendered anyway. We have been over this stuff already when I first created the base theme.

[Piedone]

So we're overriding a resource used on the admin from the theme. This is not something we should do. Especially that we shouldn't update that Bootstrap version independently of Orchard, since all of the built-in Orchard admin features depend on the Bootstrap version provided by Orchard.

So, I don't think overriding the Bootstrap resource for the admin theme is a valid use-case. If certain built-in widgets do that for the frontend, then a better approach is to override those specific shapes instead.

[sarahelsaig]

I don't want to be pushy about it, but is this really the time to consider fundamentally changing how the project works? I only wanted to add a simple lock around the existing code so all the tests in dev stop failing. I'll be leaving for today soon and don't want to be a blocker on basically every OSOCE issue in existence.

[Piedone]

I'm merging this as a hotfix, but there's a fundamental problem with the whole approach, so we need to fix that, yes. Once that's addressed, we won't need this hack either. Please work on that next.

[sarahelsaig]

I have created the issue #41.