Skip to content

Fix crash when event does not have Add method and improve message for some other internal errors #2409

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

Merged
merged 3 commits into from
Jul 2, 2024

Conversation

lostmsu
Copy link
Member

@lostmsu lostmsu commented Jul 1, 2024

What does this implement/fix? Explain your changes.

  1. Fixes crash on events with no AddMethod
  2. Adds InternalPythonnetException that can tell user what .NET type caused failure if an unexpected internal error occurs

Does this close any currently open issues?

#2405

Checklist

Check all those that are applicable and complete.

@filmor
Copy link
Member

filmor commented Jul 2, 2024

As said, while looking mildly prettier, this still renders the library completely useless in this case. I don't think there is any valid reason for us to bail out on init just because a single type is not loaded properly.

So, sure, we can merge this, but we can not claim that it fixes the reported issue.

@lostmsu
Copy link
Member Author

lostmsu commented Jul 2, 2024

@filmor the fix is the new null check in ShouldBindEvent.

The rest is to make it easier to do bug reports: the error data will now at least contain the name of the type that we failed to process.

@filmor
Copy link
Member

filmor commented Jul 2, 2024

I see. Still don't see why making ShouldBindEvent special in that it swallows the error instead of throwing is better than just behaving like this uniformly. I'd rather have none of these functions throw, but at least it fixes the reported issue.

@filmor filmor merged commit 9ebfbde into pythonnet:master Jul 2, 2024
27 checks passed
@lostmsu
Copy link
Member Author

lostmsu commented Jul 2, 2024

@filmor it does not swallow the error.

The checks on methods and fields are different. They actually check that method or field was passed, and not just null. In case of event the event itself must exist as well (e.g. not null), but some events apparently do not have a corresponding add method for handlers: AddMethod property is nullable

BTW I was unable to reproduce it with C#.

@lostmsu lostmsu deleted the InternalError branch July 2, 2024 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants