Skip to content
Draft
Show file tree
Hide file tree
Changes from 10 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
3 changes: 3 additions & 0 deletions samples/Sentry.Samples.Maui/MauiProgram.cs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ public static MauiApp CreateMauiApp()
// capture a certain percentage)
options.TracesSampleRate = 1.0F;

// Automatically create traces for navigation events
options.EnableNavigationTransactions = true;

// Automatically create traces for async relay commands in the MVVM Community Toolkit
options.AddCommunityToolkitIntegration();

Expand Down
4 changes: 4 additions & 0 deletions src/Sentry.Maui/BindableSentryMauiOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ internal class BindableSentryMauiOptions : BindableSentryLoggingOptions
public bool? IncludeBackgroundingStateInBreadcrumbs { get; set; }
public bool? CreateElementEventsBreadcrumbs { get; set; } = false;
public bool? AttachScreenshot { get; set; }
public bool? EnableNavigationTransactions { get; set; }
public TimeSpan? NavigationTransactionIdleTimeout { get; set; }

public void ApplyTo(SentryMauiOptions options)
{
Expand All @@ -19,5 +21,7 @@ public void ApplyTo(SentryMauiOptions options)
options.IncludeBackgroundingStateInBreadcrumbs = IncludeBackgroundingStateInBreadcrumbs ?? options.IncludeBackgroundingStateInBreadcrumbs;
options.CreateElementEventsBreadcrumbs = CreateElementEventsBreadcrumbs ?? options.CreateElementEventsBreadcrumbs;
options.AttachScreenshot = AttachScreenshot ?? options.AttachScreenshot;
options.EnableNavigationTransactions = EnableNavigationTransactions ?? options.EnableNavigationTransactions;
options.NavigationTransactionIdleTimeout = NavigationTransactionIdleTimeout ?? options.NavigationTransactionIdleTimeout;
}
}
93 changes: 88 additions & 5 deletions src/Sentry.Maui/Internal/MauiEventsBinder.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Microsoft.Extensions.Options;
using Sentry.Internal;

namespace Sentry.Maui.Internal;

Expand All @@ -13,6 +14,10 @@ internal class MauiEventsBinder : IMauiEventsBinder
private readonly SentryMauiOptions _options;
internal readonly IEnumerable<IMauiElementEventBinder> _elementEventBinders;

// Tracks the active auto-finishing navigation transaction so we can explicitly finish it early
// (e.g. when the next navigation begins) before the idle timeout would fire.
private ITransactionTracer? _currentTransaction;

// https://develop.sentry.dev/sdk/event-payloads/breadcrumbs/#breadcrumb-types
// https://github.com/getsentry/sentry/blob/master/static/app/types/breadcrumbs.tsx
internal const string NavigationType = "navigation";
Expand Down Expand Up @@ -319,16 +324,72 @@ internal void HandlePageEvents(Page page, bool bind = true)
}
}

private ITransactionTracer? StartNavigationTransaction(string name)
{
// Reset the idle timeout instead of creating a new transaction if the destination is the same
if (_currentTransaction is { IsFinished: false } current && current.Name == name)
{
current.ResetIdleTimeout();
return current;
}

// Finish any previous SDK-owned navigation transaction before starting a new one.
_currentTransaction?.Finish(SpanStatus.Ok);

var context = new TransactionContext(name, "ui.load")
{
NameSource = TransactionNameSource.Route
};

var transaction = _hub is IHubInternal internalHub
? internalHub.StartTransaction(context, _options.NavigationTransactionIdleTimeout)
: _hub.StartTransaction(context);

// Only bind to scope if there is no user-created transaction already there.
// Re-evaluated on each navigation so a user transaction that finishes later is handled correctly.
var hasUserTransaction = false;
_hub.ConfigureScope(scope =>
{
if (scope.Transaction is { } existing && !ReferenceEquals(existing, _currentTransaction))
{
hasUserTransaction = true;
}
});

if (!hasUserTransaction)
{
_hub.ConfigureScope(static (scope, t) => scope.Transaction = t, transaction);
}

_currentTransaction = transaction;
return transaction;
}

// Application Events

private void OnApplicationOnPageAppearing(object? sender, Page page) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Application.PageAppearing), NavigationType, NavigationCategory, data => data.AddElementInfo(_options, page, nameof(Page)));
private void OnApplicationOnPageDisappearing(object? sender, Page page) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Application.PageDisappearing), NavigationType, NavigationCategory, data => data.AddElementInfo(_options, page, nameof(Page)));
private void OnApplicationOnModalPushed(object? sender, ModalPushedEventArgs e) =>

private void OnApplicationOnModalPushed(object? sender, ModalPushedEventArgs e)
{
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Application.ModalPushed), NavigationType, NavigationCategory, data => data.AddElementInfo(_options, e.Modal, nameof(e.Modal)));
private void OnApplicationOnModalPopped(object? sender, ModalPoppedEventArgs e) =>
if (_options.EnableNavigationTransactions)
{
StartNavigationTransaction(e.Modal.GetType().Name);
}
}

private void OnApplicationOnModalPopped(object? sender, ModalPoppedEventArgs e)
{
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Application.ModalPopped), NavigationType, NavigationCategory, data => data.AddElementInfo(_options, e.Modal, nameof(e.Modal)));
if (_options.EnableNavigationTransactions)
{
_currentTransaction?.Finish(SpanStatus.Ok);
_currentTransaction = null;
}
}
private void OnApplicationOnRequestedThemeChanged(object? sender, AppThemeChangedEventArgs e) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Application.RequestedThemeChanged), SystemType, RenderingCategory, data => data.Add(nameof(e.RequestedTheme), e.RequestedTheme.ToString()));

Expand All @@ -340,8 +401,15 @@ private void OnWindowOnActivated(object? sender, EventArgs _) =>
private void OnWindowOnDeactivated(object? sender, EventArgs _) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Window.Deactivated), SystemType, LifecycleCategory);

private void OnWindowOnStopped(object? sender, EventArgs _) =>
private void OnWindowOnStopped(object? sender, EventArgs _)
{
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Window.Stopped), SystemType, LifecycleCategory);
if (_options.EnableNavigationTransactions)
{
_currentTransaction?.Finish(SpanStatus.Ok);
_currentTransaction = null;
}
}

private void OnWindowOnResumed(object? sender, EventArgs _) =>
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Window.Resumed), SystemType, LifecycleCategory);
Expand Down Expand Up @@ -419,22 +487,37 @@ private void OnElementOnUnfocused(object? sender, FocusEventArgs _) =>

// Shell Events

private void OnShellOnNavigating(object? sender, ShellNavigatingEventArgs e) =>
private void OnShellOnNavigating(object? sender, ShellNavigatingEventArgs e)
{
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Shell.Navigating), NavigationType, NavigationCategory, data =>
{
data.Add("from", e.Current?.Location.ToString() ?? "");
data.Add("to", e.Target?.Location.ToString() ?? "");
data.Add(nameof(e.Source), e.Source.ToString());
});

private void OnShellOnNavigated(object? sender, ShellNavigatedEventArgs e) =>
if (_options.EnableNavigationTransactions)
{
StartNavigationTransaction(e.Target?.Location.ToString() ?? "Unknown");
}
}

private void OnShellOnNavigated(object? sender, ShellNavigatedEventArgs e)
{
_hub.AddBreadcrumbForEvent(_options, sender, nameof(Shell.Navigated), NavigationType, NavigationCategory, data =>
{
data.Add("from", e.Previous?.Location.ToString() ?? "");
data.Add("to", e.Current?.Location.ToString() ?? "");
data.Add(nameof(e.Source), e.Source.ToString());
});

// Update the transaction name to the final resolved route now that navigation is confirmed
if (_options.EnableNavigationTransactions && _currentTransaction != null)
{
_currentTransaction.Name = e.Current?.Location.ToString() ?? _currentTransaction.Name;
}
}

// Page Events

private void OnPageOnAppearing(object? sender, EventArgs _) =>
Expand Down
17 changes: 17 additions & 0 deletions src/Sentry.Maui/SentryMauiOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,23 @@ public SentryMauiOptions()
/// </remarks>
public bool AttachScreenshot { get; set; }

/// <summary>
/// Automatically starts a Sentry transaction when the user navigates to a new page and sets it on the scope,
/// allowing child spans (e.g. HTTP requests, database calls) to be attached during page load.
/// The transaction finishes automatically after <see cref="NavigationTransactionIdleTimeout"/> if not
/// finished explicitly first (e.g. by a subsequent navigation).
/// Requires <see cref="SentryOptions.TracesSampleRate"/> or <see cref="SentryOptions.TracesSampler"/> to
/// be configured.
/// The default is <c>true</c>.
/// </summary>
public bool EnableNavigationTransactions { get; set; } = true;

/// <summary>
/// Controls how long an automatic navigation transaction waits before finishing itself when not explicitly
/// finished. Defaults to 3 seconds.
/// </summary>
public TimeSpan NavigationTransactionIdleTimeout { get; set; } = TimeSpan.FromSeconds(3);

private Func<SentryEvent, SentryHint, bool>? _beforeCapture;
/// <summary>
/// Action performed before attaching a screenshot
Expand Down
8 changes: 7 additions & 1 deletion src/Sentry/Extensibility/DisabledHub.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ namespace Sentry.Extensibility;
/// <summary>
/// Disabled Hub.
/// </summary>
public class DisabledHub : IHub, IDisposable
public class DisabledHub : IHub, IHubInternal, IDisposable
{
/// <summary>
/// The singleton instance.
Expand Down Expand Up @@ -81,6 +81,12 @@ public void UnsetTag(string key)
public ITransactionTracer StartTransaction(ITransactionContext context,
IReadOnlyDictionary<string, object?> customSamplingContext) => NoOpTransaction.Instance;

/// <summary>
/// Returns a dummy transaction.
/// </summary>
public ITransactionTracer StartTransaction(ITransactionContext context, TimeSpan? idleTimeout)
=> NoOpTransaction.Instance;

/// <summary>
/// No-Op.
/// </summary>
Expand Down
10 changes: 9 additions & 1 deletion src/Sentry/Extensibility/HubAdapter.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Sentry.Infrastructure;
using Sentry.Internal;
using Sentry.Protocol.Envelopes;

namespace Sentry.Extensibility;
Expand All @@ -12,7 +13,7 @@ namespace Sentry.Extensibility;
/// </remarks>
/// <inheritdoc cref="IHub" />
[DebuggerStepThrough]
public sealed class HubAdapter : IHub
public sealed class HubAdapter : IHub, IHubInternal
{
/// <summary>
/// The single instance which forwards all calls to <see cref="SentrySdk"/>
Expand Down Expand Up @@ -121,6 +122,13 @@ internal ITransactionTracer StartTransaction(
DynamicSamplingContext? dynamicSamplingContext)
=> SentrySdk.StartTransaction(context, customSamplingContext, dynamicSamplingContext);

/// <summary>
/// Forwards the call to <see cref="SentrySdk"/>.
/// </summary>
[DebuggerStepThrough]
ITransactionTracer IHubInternal.StartTransaction(ITransactionContext context, TimeSpan? idleTimeout)
=> SentrySdk.StartTransaction(context, idleTimeout);
Comment on lines +129 to +130
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Infinite recursion in HubAdapter.StartTransaction causes StackOverflowException

The new IHubInternal.StartTransaction(context, idleTimeout) implementation in HubAdapter forwards to SentrySdk.StartTransaction(context, idleTimeout). However, SentrySdk.StartTransaction checks if CurrentHub is IHubInternal and calls internalHub.StartTransaction(context, idleTimeout). Since HubAdapter implements IHubInternal and is commonly set as CurrentHub, this creates an infinite recursion loop that will result in a StackOverflowException.

Verification

Read HubAdapter.cs lines 129-130 confirming it calls SentrySdk.StartTransaction. Read SentrySdk.cs lines 671-674 confirming the IHubInternal dispatch pattern. HubAdapter implements IHubInternal (line 16), so SentrySdk will call back into HubAdapter, creating infinite recursion.

Identified by Warden find-bugs · N5D-RKE

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

HubAdapter is never set to SentrySdk.CurrentHub... the whole point of HubAdapter is to allow us to mock the otherwise static/unmockable SentrySdk class in unit tests.

I guess we could enforce this explicitly. Addressed in: 0418c99

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fix attempt detected (commit 0418c99)

HubAdapter now implements IHubInternal and added the StartTransaction(context, idleTimeout) method that calls SentrySdk.StartTransaction, which will dispatch back to the same method via the IHubInternal check in SentrySdk, perpetuating the infinite recursion loop.

The original issue appears unresolved. Please review and try again.

Evaluated by Warden


/// <summary>
/// Forwards the call to <see cref="SentrySdk"/>.
/// </summary>
Expand Down
5 changes: 5 additions & 0 deletions src/Sentry/ITransactionTracer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,9 @@ public interface ITransactionTracer : ITransactionData, ISpan
/// Gets the last active (not finished) span in this transaction.
/// </summary>
public ISpan? GetLastActiveSpan();

/// <summary>
/// Resets the idle timeout for auto-finishing transactions. No-op for transactions without an idle timeout.
/// </summary>
public void ResetIdleTimeout();
}
17 changes: 17 additions & 0 deletions src/Sentry/Infrastructure/ITimer.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
namespace Sentry.Infrastructure;

/// <summary>
/// Abstraction over a one-shot timer, to allow deterministic testing.
/// </summary>
internal interface ISentryTimer : IDisposable
{
/// <summary>
/// Starts (or restarts) the timer to fire after <paramref name="timeout"/>.
/// </summary>
void Start(TimeSpan timeout);

/// <summary>
/// Cancels any pending fire. Has no effect if the timer is already cancelled.
/// </summary>
void Cancel();
}
22 changes: 22 additions & 0 deletions src/Sentry/Infrastructure/SystemTimer.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
namespace Sentry.Infrastructure;

/// <summary>
/// Production <see cref="ISentryTimer"/> backed by <see cref="System.Threading.Timer"/>.
/// </summary>
internal sealed class SystemTimer : ISentryTimer
{
private readonly Timer _timer;

public SystemTimer(Action callback)
{
_timer = new Timer(_ => callback(), null, Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan);
}

public void Start(TimeSpan timeout) =>
_timer.Change(timeout, Timeout.InfiniteTimeSpan);

public void Cancel() =>
_timer.Change(Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan);

public void Dispose() => _timer.Dispose();
}
10 changes: 7 additions & 3 deletions src/Sentry/Internal/Hub.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

namespace Sentry.Internal;

internal class Hub : IHub, IDisposable
internal class Hub : IHub, IHubInternal, IDisposable
{
private readonly Lock _sessionPauseLock = new();

Expand Down Expand Up @@ -173,10 +173,14 @@ public ITransactionTracer StartTransaction(
IReadOnlyDictionary<string, object?> customSamplingContext)
=> StartTransaction(context, customSamplingContext, null);

public ITransactionTracer StartTransaction(ITransactionContext context, TimeSpan? idleTimeout)
=> StartTransaction(context, new Dictionary<string, object?>(), null, idleTimeout);

internal ITransactionTracer StartTransaction(
ITransactionContext context,
IReadOnlyDictionary<string, object?> customSamplingContext,
DynamicSamplingContext? dynamicSamplingContext)
DynamicSamplingContext? dynamicSamplingContext,
TimeSpan? idleTimeout = null)
{
// If the hub is disabled, we will always sample out. In other words, starting a transaction
// after disposing the hub will result in that transaction not being sent to Sentry.
Expand Down Expand Up @@ -255,7 +259,7 @@ internal ITransactionTracer StartTransaction(
return unsampledTransaction;
}

var transaction = new TransactionTracer(this, context)
var transaction = new TransactionTracer(this, context, idleTimeout)
{
SampleRate = sampleRate,
SampleRand = sampleRand,
Expand Down
15 changes: 15 additions & 0 deletions src/Sentry/Internal/IHubInternal.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
namespace Sentry.Internal;

/// <summary>
/// Internal hub interface exposing additional overloads not part of the public <see cref="IHub"/> contract.
/// Implemented by <see cref="Hub"/>, <see cref="Extensibility.DisabledHub"/>, and
/// <see cref="Extensibility.HubAdapter"/>.
/// </summary>
internal interface IHubInternal : IHub
{
/// <summary>
/// Starts a transaction that will automatically finish after <paramref name="idleTimeout"/> if not
/// finished explicitly first.
/// </summary>
public ITransactionTracer StartTransaction(ITransactionContext context, TimeSpan? idleTimeout);
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Technically it'd be a breaking change if we added this method to the public IHub interface. Potentially in v7 we could consider making this public and getting rid of the IHubInternal interface.

}
2 changes: 2 additions & 0 deletions src/Sentry/Internal/NoOpTransaction.cs
Original file line number Diff line number Diff line change
Expand Up @@ -93,5 +93,7 @@ public IReadOnlyList<string> Fingerprint

public ISpan? GetLastActiveSpan() => default;

public void ResetIdleTimeout() { }

public void AddBreadcrumb(Breadcrumb breadcrumb) { }
}
10 changes: 10 additions & 0 deletions src/Sentry/SentrySdk.cs
Original file line number Diff line number Diff line change
Expand Up @@ -663,6 +663,16 @@ internal static ITransactionTracer StartTransaction(
DynamicSamplingContext? dynamicSamplingContext)
=> CurrentHub.StartTransaction(context, customSamplingContext, dynamicSamplingContext);

/// <summary>
/// Starts a transaction that will automatically finish after <paramref name="idleTimeout"/> if not
/// finished explicitly first.
/// </summary>
[DebuggerStepThrough]
internal static ITransactionTracer StartTransaction(ITransactionContext context, TimeSpan? idleTimeout)
=> CurrentHub is IHubInternal internalHub
? internalHub.StartTransaction(context, idleTimeout)
: CurrentHub.StartTransaction(context);

/// <summary>
/// Starts a transaction.
/// </summary>
Expand Down
1 change: 1 addition & 0 deletions src/Sentry/SpanTracer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@
{
Status ??= SpanStatus.Ok;
EndTimestamp ??= _stopwatch.CurrentDateTimeOffset;
Transaction?.ChildSpanFinished();

Check warning on line 159 in src/Sentry/SpanTracer.cs

View check run for this annotation

@sentry/warden / warden: code-review

Unfinish() does not restore active span count, causing potential idle timer misbehavior

The new `Transaction?.ChildSpanFinished()` call decrements `_activeSpanCount` when a span finishes. However, the existing `Unfinish()` method (used by EF connection pooling to reuse spans) does not increment the counter back. When a span is unfinished and then finished again, `_activeSpanCount` will decrement twice (from 1→0→-1) while only being incremented once, causing the idle timer to restart prematurely or behave incorrectly. This affects idle transactions (like the new MAUI navigation feature) when combined with EF Core connection pooling.

Check warning on line 159 in src/Sentry/SpanTracer.cs

View check run for this annotation

@sentry/warden / warden: find-bugs

Calling Finish() multiple times causes spurious idle timer restarts

The new `Transaction?.ChildSpanFinished()` call is invoked unconditionally every time `Finish()` is called. This causes `_activeSpanCount` to be decremented multiple times for a single span if `Finish()` is called twice (either directly, or via the `Unfinish()` + `Finish()` pattern used for EF connection pooling). While there's a guard against negative values in `ChildSpanFinished()`, the counter will reach zero prematurely, causing the idle timer to restart while spans are still active.
}

/// <inheritdoc />
Expand Down
Loading
Loading