(DNN-10795) - All pages except home page return 404 #2032
Conversation
I have witnessed that occasionally on app pool recycle all,except the home page, will return 404 until the application pool is recycled a second time. I've reviewed the code & believe that the root cause of the issue is due to the fact that the code that builds the tab index, portalDepths dictionary & tabPaths dictionary is not thread safe. I can see code in the method TabIndexController.FetchTabDictionary is using SharedDictionary classes to store the tab dictionaries, however the code is not thread safe when adding the dictionaries to the cache. Therefore when multiple threads are executing the FetchTabDictionary method it's possible for an empty dictionary to be added to the cache. To resolve this issue the code has been updated so that only one thread can add the dictionaries to cache at a time.
|
The dictionary implementation is the problem? Maybe it's worth considering to replace |
|
Hi Tomaz,
The issue is not with the type of dictionary used but with the fact that multiple threads can add the dictionary to cache at the same time. The SharedDictionary used is already thread safe. It’s the code that is creating the dictionary and adding to cache that needs to be changed not the dictionary it itself.
Kind regards,
Cameron.
…Sent from my iPad
On 25 May 2018, at 11:52 pm, Tomasz Pluskiewicz ***@***.***> wrote:
The dictionary implementation is the problem? Maybe it's worth considering to replace SharedDictionary with ConcurrentDictionary
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@cameron-moore Can we get the CLA completed for this |
|
@cameron-moore The easiest way to get the CLA completed is to push another change into your fork/branch. Maybe update a code comment in this file? The other option would be to close and resubmit the fix. Sorry for the issue, we promise it's just a one time thing the first time you submit a change to the project. |
|
|
|
Hi guys, CLA has now been completed. Cheers, |
|
@cameron-moore Thank you for getting that done! We will review and get this hopefully into v9.3.0. |
|
@cameron-moore What testing has been done on this change? Do you happen to know of a quick/easy way to recreate the contention issues that happened with the old way? |
|
@mitchelsellers Because it's a thread contention issue it's quite difficult to reproduce the issue on demand because it only occurs when multiple requests are being made to the site simultaneously and the tab dictionaries are yet to be stored in cache, so it's only going to happen when the DNN instance is first loading up, or just after the dictionaries are removed from cache. When I was investigating I stepped through the code and updated the tab dictionary to be an empty collection just prior to being cached, to simulate the contention. Doing this I was able to reproduce the same behavior as we were seeing in our production DNN instance, where all requests to the site result in a 404. Changes made in this pull request were also applied to our production instance of DNN, running a couple of hundred portals. Prior to applying the change in our production environment we were seeing all pages 404ing at least once a week. Each time this would happen we'd need to recycle the application and things would start working again. We've not seen any repeats of the issue in production, where all pages 404, since this change was applied. Hope this helps. |
|
I have been testing this fix on a few installations, and it does appear to work. Following Microsoft guidance this appears to be sound as well. This issue impacts Axure installations more than others. @dnnsoftware/tag what do we think about including this in 9.3.0 given that we are doing extended testing? My vote is to include this. @daguiler and @ashishpd would like your specific weigh in as well |
There was a problem hiding this comment.
I'm not a huge fan of this PR to be honest. It combines several changes and refactoring which makes it really hard to review.
On the other hand, the issue is serious and we have production evidence that this works. Plus, I ran the unit tests and all still pass, so I'm approving.
… except home page return 404) for DNn versions 8.0.4 through 9.2.2
|
FYI - Cameron's fix here has completely resolved this problem on an 8.0.4 instance of mine that was having multiple daily outages over the past year. This should have been integrated ten years ago. Let's make it happen now. |
|
I concur on this, I'm also going to make special notes for detailed performance testing on 9.3.0. I have been able to replicate the issue in Azure and with this fix in place there is not an issue. We also need to remove the "Fixed-Dll's" from the display |
|
@mitchelsellers Feel free to delete Fixed-DLLs folder. It shouldn't have been merged. I just added that in my fork so that if people want to download a pre-compiled version of the DotNetNuke.dll, that has the changes, they can get it from my fork. It shouldn't be in the the main DnnPlatform project code. |
|
We've been experiencing this issue on multiple DNN installs, has this fix been included in DNN 9.3.0 RC? |
|
@rudgr I downloaded & took a look at the source package for 9.3.0 RC. It looks like the code has been included in the 9.3.0 RC release but just it's been missed from the release notes. |
|
It should be yes. |
|
awesome!!!!!!!!! :) |
|
I updated the release notes to call this out, apologies that we missed it @cameron-moore |
|
Sorry @cameron-moore, I completely spaced out on this PR and adding the noteworthy tag to it. It's a new addition to our process and we just need to do better next time around. Thanks for letting us know! and thanks again for taking the time to research the issue and submit a fix!! |
* DNN-10795 - All pages except home page return 404 I have witnessed that occasionally on app pool recycle all,except the home page, will return 404 until the application pool is recycled a second time. I've reviewed the code & believe that the root cause of the issue is due to the fact that the code that builds the tab index, portalDepths dictionary & tabPaths dictionary is not thread safe. I can see code in the method TabIndexController.FetchTabDictionary is using SharedDictionary classes to store the tab dictionaries, however the code is not thread safe when adding the dictionaries to the cache. Therefore when multiple threads are executing the FetchTabDictionary method it's possible for an empty dictionary to be added to the cache. To resolve this issue the code has been updated so that only one thread can add the dictionaries to cache at a time. * Updated comment to trigger Code Licence workflow. * Added compiled DLLs that include the fix for bug DNN-10795 (All pages except home page return 404) for DNn versions 8.0.4 through 9.2.2
* DNN-27517: force user logout after password changed in other place. * DNN-27517: update code by review. * DNN-27517: add host settings to control whether force logout after password changed. * NOJIRA: mark as stable. * Fixed bugs on add/remove user permissions for modules * Change algorithm to SHA1CryptoServiceProvider * Updated Issue Templates to include new RFC template and to support submissions for 9.3.0 release * Corrected structure to avoid issue linking * code review * User registration: end the response after redirect (#2511) * Initial New User Email Not Sending At Time of Creation (#2492) This is alternative way to fix above issue proposed in dnnsoftware/Dnn.AdminExperience#174 As per @sleupold , we need to move email notifications from UI to core part. Once this will be approved and merged, we can remove email notifications from UI and replace it with updated controller method to let notifications to be send to their recipients. fixes #2424 * Fix for missing SQL change (#2522) Fixes #2521 by rebuilding the PortalsDefaultLanguage view * Resolve UserProfile Loading Errors From Unsecure pages (#2494) * NOJIRA: mark as stable. * DNN-21637: add config key. * DNN-26576: prevent same-origin errors when loading popup and iframes from a secure page. * code review * Code review * (DNN-10795) - All pages except home page return 404 (#2032) * DNN-10795 - All pages except home page return 404 I have witnessed that occasionally on app pool recycle all,except the home page, will return 404 until the application pool is recycled a second time. I've reviewed the code & believe that the root cause of the issue is due to the fact that the code that builds the tab index, portalDepths dictionary & tabPaths dictionary is not thread safe. I can see code in the method TabIndexController.FetchTabDictionary is using SharedDictionary classes to store the tab dictionaries, however the code is not thread safe when adding the dictionaries to the cache. Therefore when multiple threads are executing the FetchTabDictionary method it's possible for an empty dictionary to be added to the cache. To resolve this issue the code has been updated so that only one thread can add the dictionaries to cache at a time. * Updated comment to trigger Code Licence workflow. * Added compiled DLLs that include the fix for bug DNN-10795 (All pages except home page return 404) for DNn versions 8.0.4 through 9.2.2 * Recursive read lock acquisitions not allowed (#2423) * DNN-23293 Recursive read lock acquisitions not allowed in this mode. * DNN-23293 Recursive read lock acquisitions not allowed in this mode. * Performance problems when huge number of portal aliases is in use (#2514) * DNN-27498 Performance Issues * DNN-27498 Performance Issues * minor formatting * Fixed case sensitivity issue * Added mixed cased alias support to unit tests * Fixed VanityUrl unit tests * Fixed broken LockStrategy unit tests (#2531) * Delete Fixed-DLLs folder that was added as part of PR for bug DNN-10795. (#2535) * Modules > ModuleCreator > fixed path error (#2527) * Fixed issue in ModuleCreator > Web > template.ascx * Update DNN Platform/Admin Modules/Dnn.Modules.ModuleCreator/Templates/Web/Module - HTML/template.ascx Co-Authored-By: mean2me <emanuele.colonnelli@gmail.com> * All languages are highlighted along with current - add css for languages * Log name of package when uninstalling extensions (#2557) * remove spaces * DNN-20856 After export with Content Localization site language flags disappears from pages (#2578) * Fixed parallel build (#2562) * Set active Nuget package source to All * Fixed parallel build * Inclusion of NDepend logo on the readme. (#2598) * Fix for missing SQL change Fixes #2521 by rebuilding the PortalsDefaultLanguage view * Added attribution to NDepend for the usage of their ADO tooling * Fix image/link markdown * Get language from transferred parameter (#2607) * switch encrypt method. (#2616) * DNN-29484: switch encrypt method. * NuGet Package Improvements Changes to modernize the NuGet packages published by the DNN Platform, fixes #2586. The below-submitted changes in structure have been validated by consultation with the DNN Platform Community, Microsoft Representatives, as well as validation of documentation per the published .nuspec file definition (https://docs.microsoft.com/en-us/nuget/reference/nuspec) In detail, the following items have been changed: * Migration of license information to the suggested <license> node rather than the deprecated <licenseurl> node. * Inclusion of target framework for all included .dll files, this prevents installation of the package to pre-4.5 projects protecting downstream users. * Improved package descriptions based on discussions held in the RFC regarding these improvements * Added Package-to-Package dependencies to ensure quick usage and inclusion * Updated the WebAPI and MVC packages to be holistic packages, including references to ALL needed items to develop using those patterns. All changes are current for DNN Platform version 9.3.0 or later. Packages have been built & tested locally with success. ## Suggested Usage With these improved packages, development & references should be easier. ### MVC Modules `Install-Package DotNetNuke.Web.Mvc` Should be the only needed package installation. It will install all needed dependencies, including the items necessary for WebAPI ### Modules Needing WebAPI (Not MVC) `Install-Package DotNetNuke.WebApi` Should be the only needed package for extensions not using MVC, however, needing to use WebApi for services. This will work well for WebForms or Library projects, etc. that don't need the extra references for MVC/Razor ### WebForms/Limited Modules `Install-Package DotNetNuke.Core` The most simple modules, still using the WebForms pattern can use this package for the smallest footprint For #2600 * Adjust the Source package to include changes from GitVersion (#2609) * remove old ckeditor packaging steps * Remove version to allow GitVersion to set it at build time (#2639) * Adding 09.03.01.SqlDataProvider file * Upgrade DNN to .NET Framework 4.7.2 (#2644) * Upgraded app projects to .NET Framework 4.7.2; Added missing dependency to DotNetNuke.Tests.Core as it was missing DotNetNuke.Web.Client * Removed targetframework web.config reference from Dnn.Modules.Console * Reverted unintended changes
I have witnessed that occasionally on app pool recycle all,except the home page, will return 404 until the application pool is recycled a second time.
I've reviewed the code & believe that the root cause of the issue is due to the fact that the code that builds the tab index, portalDepths dictionary & tabPaths dictionary is not thread safe. I can see code in the method TabIndexController.FetchTabDictionary is using SharedDictionary classes to store the tab dictionaries, however the code is not thread safe when adding the dictionaries to the cache. Therefore when multiple threads are executing the FetchTabDictionary method it's possible for an empty dictionary to be added to the cache.
To resolve this issue the code has been updated so that only one thread can add the dictionaries to cache at a time.