Skip to content
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.

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

Unhandled exception handler. #109806

Merged
merged 16 commits into from
Nov 26, 2024
Merged

Unhandled exception handler. #109806

merged 16 commits into from
Nov 26, 2024

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Nov 14, 2024

The managed part of #101560
It implements the public static void SetUnhandledExceptionHandler(UnhandledExceptionHandler handler); API

This covers CoreCLR and NativeAOT.
(Mono will need to be addressed separately)

@dotnet-issue-labeler dotnet-issue-labeler bot added needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners new-api-needs-documentation labels Nov 14, 2024
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

1 similar comment
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@VSadov VSadov added runtime-coreclr specific to the CoreCLR runtime area-NativeAOT-coreclr area-ExceptionHandling-coreclr area-VM-coreclr and removed new-api-needs-documentation needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners runtime-coreclr specific to the CoreCLR runtime labels Nov 14, 2024
Copy link
Contributor

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

@VSadov
Copy link
Member Author

VSadov commented Nov 15, 2024

Build browser-wasm linux Release LibraryTests is timing out at build stage. It is probably not related to the changes in this PR

@VSadov VSadov marked this pull request as ready for review November 15, 2024 01:35
@VSadov VSadov requested review from jkotas and janvorli and removed request for MichalStrehovsky November 15, 2024 01:35
@VSadov
Copy link
Member Author

VSadov commented Nov 15, 2024

I think this is ready for review.

@VSadov
Copy link
Member Author

VSadov commented Nov 15, 2024

(Mono will need to be addressed separately)

That is because I am far less familiar with where the relevant pieces are in Mono, so I'd like to get CoreCLR/NativeAOT and tests settled.
I may need help with Mono.

#include <pthread.h>
#endif // _WIN32

// Work around typedef redefinition: platformdefines.h defines error_t
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the platformdefines.h should be fixed instead to not to define the error_t and include the errno.h instead.

public class PInvokeRevPInvokeUnhandled
{
[DllImport("ForeignThreadRevPInvokeUnhandled")]
public static extern void InvokeCallback(MyCallback callback);
Copy link
Member

Choose a reason for hiding this comment

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

I cannot see this being used in this test

Copy link
Member Author

Choose a reason for hiding this comment

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

I just copied the file between two tests. One uses InvokeCallback another uses InvokeCallbackOnNewThread. I can remove redundant ones.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@VSadov
Copy link
Member Author

VSadov commented Nov 25, 2024

The failure in x86 tests is #110127

@VSadov
Copy link
Member Author

VSadov commented Nov 26, 2024

/ba-g #110127

@VSadov VSadov merged commit 17cb826 into dotnet:main Nov 26, 2024
138 of 140 checks passed
@VSadov VSadov deleted the unh branch November 26, 2024 00:03

namespace System.Runtime.ExceptionServices
{
public delegate bool UnhandledExceptionHandler(System.Exception exception);
Copy link
Contributor

@teo-tsirpanis teo-tsirpanis Nov 26, 2024

Choose a reason for hiding this comment

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

@VSadov the approved API shape used Func<Exception, bool> instead of a custom delegate.

Copy link
Member

Choose a reason for hiding this comment

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

@VSadov the approved API shape used Func<Exception, bool> instead of a custom delegate.

@VSadov do you have plans to address this? We cannot check in new unapproved public APIs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Opened #110254.

Copy link
Member Author

Choose a reason for hiding this comment

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

do you have plans to address this? We cannot check in new unapproved public APIs.

Yes. It was an oversight.
The approved shape was the same as in proposal except for this part. I missed that there was a diff from the proposal.

mikelle-rogers pushed a commit to mikelle-rogers/runtime that referenced this pull request Dec 10, 2024
* unhandled exceptions in Finalizers (Core and AOT)

* catch in threadpool

* handle in threads

* trivial tests

* HandlerThrows

* HandlerRefuses

* NoEffectInMainThread

* Rev PInvoke tests

* make all pri-0 and disable on mono

* use ?.

* whitespaces

* rename unhandledHandler-->UnhandledExceptionHandler

* PR feedback

* define error_t as int

* ref/System.Runtime formatting

* Update src/tests/baseservices/exceptions/UnhandledExceptionHandler/UnhandledTrivial.cs

---------

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants