Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,9 @@
builder.AddNotificationAsyncHandler<RuntimeUnattendedUpgradeNotification, UnattendedUpgrader>();
builder.AddNotificationAsyncHandler<RuntimePremigrationsUpgradeNotification, PremigrationUpgrader>();

// Database availability check.
builder.Services.AddUnique<IDatabaseAvailabilityCheck, DefaultDatabaseAvailabilityCheck>();

Check warning on line 95 in src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.CoreServices.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (main)

❌ Getting worse: Large Method

AddCoreInitialServices increases from 119 to 120 lines of code, threshold = 70. Large functions with many lines of code are generally harder to understand and lower the code health. Avoid adding more lines to this function.
// Add runtime mode validation
builder.Services.AddSingleton<IRuntimeModeValidationService, RuntimeModeValidationService>();
builder.RuntimeModeValidators()
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
using Microsoft.Extensions.Logging;

namespace Umbraco.Cms.Infrastructure.Persistence;

/// <summary>
/// Checks if a configured database is available on boot using the default method of 5 attempts with a 1 second delay between each one.
/// </summary>
internal class DefaultDatabaseAvailabilityCheck : IDatabaseAvailabilityCheck
{
private const int NumberOfAttempts = 5;
private const int DefaultAttemptDelayMilliseconds = 1000;

private readonly ILogger<DefaultDatabaseAvailabilityCheck> _logger;

/// <summary>
/// Initializes a new instance of the <see cref="DefaultDatabaseAvailabilityCheck"/> class.
/// </summary>
/// <param name="logger"></param>
public DefaultDatabaseAvailabilityCheck(ILogger<DefaultDatabaseAvailabilityCheck> logger) => _logger = logger;

/// <summary>
/// Gets or sets the number of milliseconds to delay between attempts.
/// </summary>
/// <remarks>
/// Exposed for testing purposes, hence settable only internally.
/// </remarks>
public int AttemptDelayMilliseconds { get; internal set; } = DefaultAttemptDelayMilliseconds;

/// <inheritdoc/>
public bool IsDatabaseAvailable(IUmbracoDatabaseFactory databaseFactory)
{
bool canConnect;
for (var i = 0; ;)
{
canConnect = databaseFactory.CanConnect;
if (canConnect || ++i == NumberOfAttempts)
{
break;
}

if (_logger.IsEnabled(LogLevel.Debug))
{
_logger.LogDebug("Could not immediately connect to database, trying again.");
}

// Wait for the configured time before trying again.
Thread.Sleep(AttemptDelayMilliseconds);
}

return canConnect;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
namespace Umbraco.Cms.Infrastructure.Persistence;

/// <summary>
/// Checks if a configured database is available on boot.
/// </summary>
public interface IDatabaseAvailabilityCheck
{
/// <summary>
/// Checks if the database is available for Umbraco to boot.
/// </summary>
/// <param name="databaseFactory">The <see cref="IUmbracoDatabaseFactory"/>.</param>
/// <returns>
/// A value indicating whether the database is available.
/// </returns>
bool IsDatabaseAvailable(IUmbracoDatabaseFactory databaseFactory);
}
56 changes: 32 additions & 24 deletions src/Umbraco.Infrastructure/Runtime/RuntimeState.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using Microsoft.Extensions.DependencyInjection;

Check notice on line 1 in src/Umbraco.Infrastructure/Runtime/RuntimeState.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (main)

✅ Getting better: Overall Code Complexity

The mean cyclomatic complexity decreases from 4.75 to 4.25, threshold = 4. This file has many conditional statements (e.g. if, for, while) across its implementation, leading to lower code health. Avoid adding more conditionals.
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using Umbraco.Cms.Core;
Expand Down Expand Up @@ -31,6 +31,7 @@
private readonly IConflictingRouteService _conflictingRouteService = null!;
private readonly IEnumerable<IDatabaseProviderMetadata> _databaseProviderMetadata = null!;
private readonly IRuntimeModeValidationService _runtimeModeValidationService = null!;
private readonly IDatabaseAvailabilityCheck _databaseAvailabilityCheck = null!;

/// <summary>
/// The initial <see cref="RuntimeState"/>
Expand All @@ -46,6 +47,7 @@
/// <summary>
/// Initializes a new instance of the <see cref="RuntimeState" /> class.
/// </summary>
[Obsolete("Please use the constructor taking all parameters. Scheduled for removal in Umbraco 18.")]
public RuntimeState(
IOptions<GlobalSettings> globalSettings,
IOptions<UnattendedSettings> unattendedSettings,
Expand All @@ -56,6 +58,34 @@
IConflictingRouteService conflictingRouteService,
IEnumerable<IDatabaseProviderMetadata> databaseProviderMetadata,
IRuntimeModeValidationService runtimeModeValidationService)
: this(
globalSettings,
unattendedSettings,
umbracoVersion,
databaseFactory,
logger,
packageMigrationState,
conflictingRouteService,
databaseProviderMetadata,
runtimeModeValidationService,
StaticServiceProvider.Instance.GetRequiredService<IDatabaseAvailabilityCheck>())
{
}

/// <summary>
/// Initializes a new instance of the <see cref="RuntimeState" /> class.
/// </summary>
public RuntimeState(
IOptions<GlobalSettings> globalSettings,
IOptions<UnattendedSettings> unattendedSettings,
IUmbracoVersion umbracoVersion,
IUmbracoDatabaseFactory databaseFactory,
ILogger<RuntimeState> logger,
PendingPackageMigrations packageMigrationState,
IConflictingRouteService conflictingRouteService,
IEnumerable<IDatabaseProviderMetadata> databaseProviderMetadata,
IRuntimeModeValidationService runtimeModeValidationService,
IDatabaseAvailabilityCheck databaseAvailabilityCheck)
{
_globalSettings = globalSettings;
_unattendedSettings = unattendedSettings;
Expand All @@ -66,6 +96,7 @@
_conflictingRouteService = conflictingRouteService;
_databaseProviderMetadata = databaseProviderMetadata;
_runtimeModeValidationService = runtimeModeValidationService;
_databaseAvailabilityCheck = databaseAvailabilityCheck;
}

/// <inheritdoc />
Expand Down Expand Up @@ -242,7 +273,7 @@
{
try
{
if (!TryDbConnect(databaseFactory))
if (_databaseAvailabilityCheck.IsDatabaseAvailable(databaseFactory) is false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't the stuff below this just retry until it can get their data from the database? With the current logic you're bound to run into errors regardless (e.g., if the database is restarted immediately after this check).

{
return UmbracoDatabaseState.CannotConnect;
}
Expand Down Expand Up @@ -305,27 +336,4 @@
}
return CurrentMigrationState != FinalMigrationState;
}

private bool TryDbConnect(IUmbracoDatabaseFactory databaseFactory)
{
// anything other than install wants a database - see if we can connect
// (since this is an already existing database, assume localdb is ready)
bool canConnect;
var tries = 5;
for (var i = 0; ;)
{
canConnect = databaseFactory.CanConnect;
if (canConnect || ++i == tries)
{
break;
}
if (_logger.IsEnabled(Microsoft.Extensions.Logging.LogLevel.Debug))
{
_logger.LogDebug("Could not immediately connect to database, trying again.");
}
Thread.Sleep(1000);
}

return canConnect;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
using Microsoft.Extensions.Logging.Abstractions;
using Moq;
using NUnit.Framework;
using Umbraco.Cms.Core.Install.Models;
using Umbraco.Cms.Infrastructure.Persistence;
using Umbraco.Cms.Persistence.SqlServer.Services;

namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Persistence;

[TestFixture]
public class DefaultDatabaseAvailabilityCheckTests
{
[Test]
public void IsDatabaseAvailable_WithDatabaseUnavailable_ReturnsFalse()
{
var mockDatabaseFactory = new Mock<IUmbracoDatabaseFactory>();
mockDatabaseFactory
.Setup(x => x.CanConnect)
.Returns(false);

var sut = CreateDefaultDatabaseAvailabilityCheck();
var result = sut.IsDatabaseAvailable(mockDatabaseFactory.Object);
Assert.IsFalse(result);
}

[Test]
public void IsDatabaseAvailable_WithDatabaseImmediatelyAvailable_ReturnsTrue()
{
var mockDatabaseFactory = new Mock<IUmbracoDatabaseFactory>();
mockDatabaseFactory
.Setup(x => x.CanConnect)
.Returns(true);

var sut = CreateDefaultDatabaseAvailabilityCheck();
var result = sut.IsDatabaseAvailable(mockDatabaseFactory.Object);
Assert.IsTrue(result);
}

[TestCase(5, true)]
[TestCase(6, false)]
public void IsDatabaseAvailable_WithDatabaseImmediatelyAvailableAfterMultipleAttempts_ReturnsExpectedResult(int attemptsUntilConnection, bool expectedResult)
{
if (attemptsUntilConnection < 1)
{
throw new ArgumentException($"{nameof(attemptsUntilConnection)} must be greater than or equal to 1.", nameof(attemptsUntilConnection));
}

var attemptResults = new Queue<bool>();
for (var i = 0; i < attemptsUntilConnection - 1; i++)
{
attemptResults.Enqueue(false);
}

attemptResults.Enqueue(true);

var mockDatabaseFactory = new Mock<IUmbracoDatabaseFactory>();
mockDatabaseFactory
.Setup(x => x.CanConnect)
.Returns(attemptResults.Dequeue);

var sut = CreateDefaultDatabaseAvailabilityCheck();
var result = sut.IsDatabaseAvailable(mockDatabaseFactory.Object);
Assert.AreEqual(expectedResult, result);
}

private static DefaultDatabaseAvailabilityCheck CreateDefaultDatabaseAvailabilityCheck()
=> new(new NullLogger<DefaultDatabaseAvailabilityCheck>())
{
AttemptDelayMilliseconds = 1 // Set to 1 ms for faster tests.
};
}