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

Improve loader robustness #2303

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Improve loader robustness #2303

wants to merge 3 commits into from

Conversation

sefgit
Copy link

@sefgit sefgit commented Dec 26, 2023

What does this implement/fix? Explain your changes.

Ignore and print loader error and proceed with loading if possible.

There are cases where
"import " in python failed even when clr.AddReference() succeeded, especially when loading "manipulated (probably protected/obfuscated)" C# DLL.

Does this close any currently open issues?

unknown

Any other comments?

Checklist

Check all those that are applicable and complete.

  • Make sure to include one or more tests for your change
  • If an enhancement PR, please create docs and at best an example
  • Ensure you have signed the .NET Foundation CLA
  • Add yourself to AUTHORS
  • Updated the CHANGELOG

Ignore and print loader error and proceed with loading if possible.
PR requirements
@filmor
Copy link
Member

filmor commented Dec 26, 2023

Can you give an example and a testcase for this? If we fail to load types, I'd rather have clr.AddReference error out entirely, and it should certainly not write to stderr.

@sefgit
Copy link
Author

sefgit commented Dec 26, 2023

Can you give an example and a testcase for this? If we fail to load types, I'd rather have clr.AddReference error out entirely, and it should certainly not write to stderr.

There is no error in calling clr.AddReference().
It's just not possible to call "import " afterwards.
I can't share the dll here publicly but I could provide you its URL thru private message if possible.

@lostmsu
Copy link
Member

lostmsu commented Dec 26, 2023

Console is not a proper way to report errors. Either AddReference should throw or (if that can't be allowed due to backward compatibility concerns if it would fail too often) we should provide a generic DLL loading error handler (e.g. event OnLoadError). Dumping such errors to Debug.WriteLine or Python's warning module (undesirable due to new dependency) would also be acceptable, but a generic handler would still be needed.

@filmor
Copy link
Member

filmor commented Dec 26, 2023

There are direct C-API functions to issue warnings: https://docs.python.org/3/c-api/exceptions.html#issuing-warnings

@sefgit
Copy link
Author

sefgit commented Dec 27, 2023

A verbose flag to enable/disable warning/error console print would be an alternative
Silent failure is totally confusing amd desperating to handle with.
Maybe:
clr.VERBOSE = True
or even introduce verbose level ?

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.

None yet

3 participants