umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
MIT License
4.45k stars 2.68k forks source link

This document is published but its url cannot be routed #9514

Closed sbosell closed 3 years ago

sbosell commented 3 years ago

Sometimes a node in umbraco will be in a state of published but not routable when this should not be allowed to occur. Umbraco in this case throws a 404 for that route.

There are several cases of this being reported as well as other issues reported here (duplicate)

Related to issue #7575

Umbraco version

I am seeing this issue on Umbraco version: 8.6.2, 8.7.x, and 8.9.1

Reproduction

I have a process that syncs data in the backend every 30 minutes to a node's descendants and then the content api calls SaveAndPublish and/or SaveAndPublishBranch (tested with both in the sync). The sync uses the content api via a recurring task, all standard features of Umbraco. In my case we have no cultures defined and only one root site. The most basic Umbraco setup. Others are reporting the same issue so it may have nothing to do with the Content Api.

About 1 time per day the root node of the sync will be in this state of the url can't be routed and it is random in that it only occurs every once in a while which leads me to believe there is a locking mechanism with how the cache works. This should never happen, there should be no code path that unpublished or deroutes a node like this in the save and publish.

Hosted on Azure in a single site (not load balanced) with all the appropriate settings set via the Umbraco Documentation.

Bug summary

A node becomes not routable when this should never happen.

Specifics

I can privately provide a URL

Steps to reproduce

It happens randomly

Expected result

A valid published node in Umbraco should NEVER be in a state of the URL not being routable.

Actual result

Umbraco backend reports the URL not being routable and the node is effectively unpublished.


_This item has been added to our backlog AB#9840_

kimschurmann commented 3 years ago

We have a couple of places where we use Current.UmbracoContext - is that a problem too? But it is not pinned as a static though!

That should be okay. You do not have any custom content finders? If you have, please ensure these are not introducing the issue.

We only have one and it is very simple - and do not introduce this issue.

sbosell commented 3 years ago

Who in this thread has no custom packages. I don't and posted code and got zero response if that could be the issue.

vaags commented 3 years ago

For us this issue was really about missing languages under "culture and hostnames" on the root-node. The published document had a language that was not listed, and was therefore not being routed.

Shazwazza commented 3 years ago

Thanks @vaags, good find.

If everyone can please check that this might be your issue and let us know that would be appreciated.


@kimschurmann Current.UmbracoContext should be ok so long as you are using it in a request thread (still best to use DI wherever possible) but please definitely make sure you are never assigning the result of this to a static variable anywhere. This will cause problems.


I'll re-state my above comment in case it was overlooked:

Can everyone make sure you are not pinning non-singletons to the root as statics. This will absolutely cause all sorts of problems like this one.

Example: do not ever store the UmbracoContext or HttpContext into a static variable

My advice is to just stop using statics all together. It will make your code safer, more manageable, makes spaghetti code far more difficult to make, prevents memory leaks and prevents problems like there where a non-singleton object is pinned to the root as a singleton. Avoid statics and please use dependency injection it will make your code so much cleaner, easier to manage and you don't run into these problems.

If anyone exhibiting this issue can report back if this is their underlying problem it will be very helpful.


@sbosell

RE: https://github.com/umbraco/Umbraco-CMS/issues/9514#issuecomment-785946163 Your usage of using (var umb = _umbracoContextFactory.EnsureUmbracoContext()) is correct, but what are you doing with the umb instance? Does your ProcessLocation relying on Current.UmbracoContext?? You should be passing the actual UmbracoContext around and not relying on Current.UmbracoContext. Since this is a background thread you should also be wrapping all of this in a custom Scope. So in PerformRun you should have this wrapped around everything:

using (var scope = _scopeProvider.CreateScope()) 
{
   // TODO: do stuff

   // Complete the scope, this completes the transaction
   // If an exception occurs anywhere above then this isn't called and the whole trans is rolled back.
   scope.Complete(); 
}

I was just re-reading all msgs here and revisiting this one: https://github.com/umbraco/Umbraco-CMS/issues/9514#issuecomment-768255937 Are pages that de-route only the ones related to /locations? That would be interesting for 2 reasons:

Zweben commented 3 years ago

@Shazwazza:

I'll re-state my above comment in case it was overlooked:

Can everyone make sure you are not pinning non-singletons to the root as statics. This will absolutely cause all sorts of problems like this one.

Example: do not ever store the UmbracoContext or HttpContext into a static variable

My advice is to just stop using statics all together. It will make your code safer, more manageable, makes spaghetti code far more difficult to make, prevents memory leaks and prevents problems like there where a non-singleton object is pinned to the root as a singleton. Avoid statics and please use dependency injection it will make your code so much cleaner, easier to manage and you don't run into these problems.

If anyone exhibiting this issue can report back if this is their underlying problem it will be very helpful.

Reporting back with an update on my comment from 15 days ago. My code had static references to UmbracoContext because I didn't know better at the time. Although admittedly I haven't done a lot of work on the site in the past couple weeks, since removing those and switching everything to use dependency injection, I have not observed any further issues.

Just wondering, is it technically possible for the CMS to detect things like stale references to UmbracoContext, and provide users with a warning? Having a "common pitfalls" page is good, but I'd imagine that an active warning for things like that, perhaps in a health check, could do wonders for perceived stability.

kimschurmann commented 3 years ago

FYI:

We took the risk and upgraded to 8.11.1 and made extensive test and monitoring on our test environment. We did not find any issues at all and have now upgraded in our production environment too.

Although upgrading from 8.5.5 to 8.11.1 made content unavailable until we wiped the internal caches from the backoffice - this should probably be done automatically as a part of the upgrade?

vaags commented 3 years ago

Although upgrading from 8.5.5 to 8.11.1 made content unavailable until we wiped the internal caches from the backoffice - this should probably be done automatically as a part of the upgrade?

I also exprerienced this. See #9378

Shazwazza commented 3 years ago

@Zweben

Thanks for responding.

Just wondering, is it technically possible for the CMS to detect things like stale references to UmbracoContext, and provide users with a warning? Having a "common pitfalls" page is good, but I'd imagine that an active warning for things like that, perhaps in a health check, could do wonders for perceived stability.

It's not. It could be done with static code analysis tools but not at runtime by the CMS.


I'm closing this issue now as there's been no further responses for my questions and some folks here have had this issue resolved by one or more of the solutions proposed in the above discussion.

FransdeJong commented 3 years ago

I ran into this issue on a site that went live yesterday with Umbraco 8.12.2. There is one specific page as far as I know that is dropping from cache with the notification that the Url cannot be routed This now happened 4 times with random time intervals. The front-end shows a 404 page.

@Shazwazza I checked the static context and we have nothing like that in place. This page is a simple page that uses no route hijacking, no contentfinders or other magic solutions that would cause this to happen.

This is a cloud project so I'm able to add you or anyone else to the project if you want to investigate this further. If you need any information please let me know.

dawoe commented 3 years ago

@Shazwazza @FransdeJong

I noticed this as well on Umbraco Cloud several times after a deployment. I guess this happens when a doctype changes. Will keep monitoring it.

Dave

FransdeJong commented 3 years ago

@dawoe In this case I didn't push anything. It just drops from the cache. Reloading memory cache fixes it.

nathanwoulfe commented 3 years ago

Apologies for resurrecting an old thread, but I'm noticing something which may be related -

This error just occurred right now.

image

Note the node name => Locations

Any time I try to create a node named Locations, I get a routing error:

This document is published but its URL would collide with content /site.url/parentNodeName (id=1234)

Doesn't matter where in the site I create the node, the error is always identical, referencing the parent node.

Might just be a coincidence, but it's an odd one if so...

@sbosell @Jay-umbr @FransdeJong @dawoe

Nicholas-Westby commented 3 years ago

@nathanwoulfe I wonder if it's related to the fact that your node name matches your template name (and your doctype name for that matter).

nathanwoulfe commented 3 years ago

@Nicholas-Westby nah, not in this case:

Parent node is the same type and template, name is About.

Haven't tried creating Locations under other node types and using different node types, will do and see what happens...

Update: seems to cause the issue regardless of type and location in the tree, if the node name is Locations, it can't be routed. There's a bit of irony in not being able to find a node named Locations 😄

Nicholas-Westby commented 3 years ago

@nathanwoulfe I see, so you ran into the same problem when you created a page called "Locations" that was using the doctype and template "Super textpage"? I could see it still being a problem unless you deleted the "Locations" template entirely (because the node name would still match SOME template, even though it isn't the template used on that node).

Though, your counter example of having a page called "About" with a doctype called "About" and a template called "About" seems to indicate the naming of the doctype and template are not an issue.

Also interesting that it happens anywhere in the tree. I can't remember if templates can be served up via subnodes, though I remember that at least they can be served up via the root (e.g., site.com/locations when Locations.cshtml exists).

nathanwoulfe commented 3 years ago

Ah, you're right - it will be catching Locations as an alternate template for the parent page, which is a bit odd since the Locations template isn't allowed for anything other than the Locations content type (will double check that, but that should be the case).

Interestingly not an issue in v7, only surfaced as part of a V8 upgrade.

jomehmet commented 2 years ago

Got this error, almost all pages ended up in 404.

Looking in the logs after this happened to us, I don't find any clear errors, but when I investigate the source code on github I wonder if there is a logical error. If you have a look in the snippet from the logs, it says Resume document cache (reload:False) which is strange, since in the source code we can se reload:True would trigger pageRefresher?.RefreshAll() that I excepect would refresh the cache, like when we go to Settings-> Published Status -> Reload Memory Cache

Wouldn't it help to let the method SuspendDocumentCache which is shown in the logs to be executed before ResumeDocumentCache to set s_tried=true

I can't figure out where this code is consumed from Github, but would apriciate if someone could verify if this is a logical error.

image