Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handled flushing of cache instruction messages when created from handling events in a background thread #11757

Conversation

AndyButland
Copy link
Contributor

This PR relates to this issue raised on the Deploy tracker. The problem and cause is well described there, and I've replicated with a load balanced setup and found the same behaviour.

It looks like we need to fix this in CMS rather than Deploy though, as Deploy is raising the expected events following a content deployment operation.

What's going wrong though is that when the events are handled in DistributedCacheBinder, we ensure an Umbraco context, using the HttpContext if available, but creating one if not. For the events triggered from Deploy, we don't have an HttpContext so one is created. When the events related to cache instructions are handled, the BatchedDatabaseServerMessenger is used to create a batch of instructions, stored in HttpContext.Items - which is on our created HttpContext.

The flushing of the messages though - and the writing of the cache instruction records to the database - happens at the end of an Umbraco request, which will have an HttpContext, but a different one to the one we created when handling the events. And hence it doesn't find any messages in the HttpContext.Items to write.

What I've done is to add a dependency to DistributedCacheBinder on the BatchedDatabaseServerMessenger interface, IServerMessenger. And then called an explicit "flush" operation using the created HttpContext. To do that I had to create some method overloads to allow the passing in of an HttpContextBase instance.

This looks to resolve the problem. The only concern I've got really is this additional dependency, and whether from an architecture/semantic point of view the DistributedCacheBinder should really have to "know" about flushing server messages. But maybe it's OK, and hopefully there's a clear enough comment about why it seems to be necessary and has been added.

To Test (there's a bit of setup needed to prepare this, so not sure if it's necessary to redo or just review what I did):

  • Set up a local load balanced Umbraco environment. I used some steps from the training course to do this, but essentially it needs:
    • A single Umbraco CMS web application to use as a staging environment.
    • Two Umbraco CMS web application to use as live environments
    • Set all these up with IIS and host headers
    • Configure Umbraco Deploy between the staging application and one of the live ones:
      • Install from NuGet UmbracoDeploy.OnPrem
      • Make the necessary updates to web.config (the custom section, the reference to the Deploy settings, and the API key)
      • Add and configure config\UmbracoDeploy.config (referencing one of the live environments from staging)
      • Add and configure config\UmbracoDeploy.Settings.config (can be empty)
    • Create a doc type with template and a single property in stage
      • The template should just write out the value of the property
    • Copy the .cshtml file created to both live environments
    • Copy the .uda files created to one of the live environments and run a Deploy extraction
    • Create a content item and deploy it
      • Make sure the template renders the correct value in all environments
    • Make a change to the content and deploy it
      • Before the change in this PR, you would see only the first live environment displaying the updated value
      • With the change in this PR, you should see both environments updating,

…xt, and used this in DistributedCacheBinder to ensure messages created are flushed from the same context.
Copy link
Member

@bergmania bergmania left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a single comment, otherwise it looks good..

@AndyButland AndyButland merged commit fd1de8e into v8/dev Dec 20, 2021
@AndyButland AndyButland deleted the v8/bugfix/handle-batched-server-messages-on-background-thread branch December 20, 2021 10:43
@GeorgeEllerby96
Copy link

Hello all,

Does anyone know roughly when this fix will be available for release?

Thanks!

@nul800sebastiaan
Copy link
Member

@GeorgeEllerby96 This will be part of 8.18.0, for which we don't have a solid date yet but we're currently looking to try to get it out in about 2 weeks.

@GeorgeEllerby96
Copy link

@nul800sebastiaan many thanks for the update!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants