Skip to content

Throw NotSupportedException when using SetErrorStatusOnException in Mono Runtime and Native AOT environment. #5374

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.

Sign up for GitHub

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

Merged
merged 23 commits into from
Mar 2, 2024
Merged

Throw NotSupportedException when using SetErrorStatusOnException in Mono Runtime and Native AOT environment. #5374

Changes from 5 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
9d179cd
initial commit
Yun-Ting Feb 21, 2024
0e9d7a9
sequence
Yun-Ting Feb 21, 2024
5299caf
Merge branch 'main' into yunl/ExceptionProcessor
Yun-Ting Feb 21, 2024
a46a246
using
Yun-Ting Feb 21, 2024
9f4b46e
Merge branch 'yunl/ExceptionProcessor' of https://github.com/Yun-Ting…
Yun-Ting Feb 21, 2024
2b9f993
using
Yun-Ting Feb 21, 2024
bbf07b0
Merge branch 'main' into yunl/ExceptionProcessor
Yun-Ting Feb 28, 2024
72ee3b7
addressed comments
Yun-Ting Feb 29, 2024
70171d5
spelling
Yun-Ting Feb 29, 2024
7447ccd
Merge branch 'main' into yunl/ExceptionProcessor
Yun-Ting Feb 29, 2024
2708aff
comments
Yun-Ting Feb 29, 2024
4a44a7a
addressed comments
Yun-Ting Feb 29, 2024
957c236
Merge branch 'main' into yunl/ExceptionProcessor
Yun-Ting Feb 29, 2024
efb3d67
changelog
Yun-Ting Feb 29, 2024
7090344
spelling
Yun-Ting Feb 29, 2024
b7864b0
comments
Yun-Ting Feb 29, 2024
cf3ac04
lint
Yun-Ting Feb 29, 2024
b6648f6
update
Yun-Ting Mar 1, 2024
a69e9a0
tweak
Yun-Ting Mar 1, 2024
6c57649
Merge branch 'main' into yunl/ExceptionProcessor
Yun-Ting Mar 1, 2024
6e2e538
Update src/OpenTelemetry/Trace/Builder/TracerProviderBuilderExtension…
Yun-Ting Mar 1, 2024
0a03b52
Merge branch 'main' into yunl/ExceptionProcessor
Yun-Ting Mar 1, 2024
8db5d36
Update src/OpenTelemetry/CHANGELOG.md
utpilla Mar 2, 2024
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
14 changes: 12 additions & 2 deletions src/OpenTelemetry/Trace/ExceptionProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@
// SPDX-License-Identifier: Apache-2.0

using System.Diagnostics;
#if NET6_0_OR_GREATER || NETFRAMEWORK
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
#if !NET6_0_OR_GREATER && !NETFRAMEWORK
#else
using System.Linq.Expressions;
using System.Reflection;
using System.Runtime.InteropServices;
#endif

namespace OpenTelemetry.Trace;
Expand All @@ -19,7 +22,14 @@ internal sealed class ExceptionProcessor : BaseProcessor<Activity>
public ExceptionProcessor()
{
#if NET6_0_OR_GREATER || NETFRAMEWORK
this.fnGetExceptionPointers = Marshal.GetExceptionPointers;
if (RuntimeFeature.IsDynamicCodeSupported)
Copy link

@jkotas jkotas Feb 21, 2024

Choose a reason for hiding this comment

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

Suggested change
if (RuntimeFeature.IsDynamicCodeSupported)
bool getExceptionPointersSupported = true;
try { Marshal.GetExceptionPointers(); } catch { getExceptionPointersSupported = false; }
if (!getExceptionPointersSupported)
throw new throw new NotSupportedException($"'{typeof(Marshal).FullName}.GetExceptionPointers' is not supported.");

Instead of calling RuntimeFeature.IsDynamicCodeSupported, can this just call the API to see whether it is supported?

Note that this API is also not supported in Mono (irrespective of whether IsDynamicCodeSupported is true or false), so it would cover the Mono.

Copy link
Contributor Author

@Yun-Ting Yun-Ting Feb 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

what would it take to actually support the API in native AOT?

Yes, it should be possible to simulated the API in native AOT. The API would need to allocate a piece of memory with the same lifetime as the current exception in flight (if it was not previously allocated), fill it with gunk, and return the pointer to it. The "new" exception handling that is being enabled for CoreCLR and that is not internally based on SEH proves that it is possible.

(Also, as I have said earlier - introducing a new API that is explicitly designed for this scenario would be the proper solution.)

Thank you! I agree above is the proper way to handle the situation. Shall I log this feature request in the runtime repo?

Copy link

@jkotas jkotas Feb 21, 2024

Choose a reason for hiding this comment

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

Shall I log this feature request in the runtime repo?

Yes, please.

Copy link
Contributor Author

@Yun-Ting Yun-Ting Feb 24, 2024

Choose a reason for hiding this comment

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

I've filed a request to implement the API in runtime: dotnet/runtime#98878.
Chatted with @eerhardt, @jkotas and @reyang offline.

While the above API is being implemented, we may do the followings to avoid crashing the consumer's app:

  1. Annotate SetErrorStatusOnException() with [RequiresDynamicCode] to serve as a compile time warning for the consumer. One caveat of this approach is IsDynamicCode does not really explain the nature of this issue and may further confuse the consumer.
  2. Use try/catch to swallow the exception if users hit System.PlatformNotSupportedException upon calling Marshal.GetExceptionPointer(). Store the state in ExceptionProcessor() and dump the Message "Exception processor not supported" inside OnEnd(Activity activity).

Not sure which one is a better stopgap of this issue?
cc @MichalStrehovsky, @agocke

Copy link
Contributor Author

@Yun-Ting Yun-Ting Feb 28, 2024

Choose a reason for hiding this comment

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

Discussed in today's SIG meeting.
I'll go with adding the compile time warning as was mentioned in 1. above.
Plus, guarding the extension method

opentelemetry-dotnet/src/OpenTelemetry/Trace/Builder/TracerProviderBuilderSdk.cs

Line 144 in e7cbbbb

public TracerProviderBuilder SetErrorStatusOnException(bool enabled)

with a try/catch on Marshal.GetExceptionPointers to avoid adding useless Processor at runtime (if user ever decided to suppress the compile time AOT warning.)

Copy link
Contributor

@utpilla utpilla Feb 28, 2024

Choose a reason for hiding this comment

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

There is no need to add try/catch block. We just have to call Marshal.GetExceptionPointers() once in the ExceptionProcessor ctor and rethrow the exception with our custom message.

@CodeBlanch Just to confirm this is what you suggested, right?

Copy link
Member

@CodeBlanch CodeBlanch Feb 28, 2024

Choose a reason for hiding this comment

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

I'm OK with either re-throwing or just silently failing & not adding the processor. Probably re-throwing is better to alert the users 🤔

Copy link
Member

@vishweshbankwar vishweshbankwar Feb 28, 2024

Choose a reason for hiding this comment

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

+1 on re-throwing the exception at start-up.

{
throw new NotSupportedException($"'{typeof(Marshal).FullName}.GetExceptionPointers' is not supported when running native AOT.");
Copy link
Member

@reyang reyang Feb 21, 2024

Choose a reason for hiding this comment

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

Is it possible to make this a build time error? I think that will help the developers to discover the issue earlier.

}
else
{
this.fnGetExceptionPointers = Marshal.GetExceptionPointers;
}
#else
// When running on netstandard or similar the Marshal class is not a part of the netstandard API
// but it would still most likely be available in the underlying framework, so use reflection to retrieve it.
Expand Down