search

dotnet / maintenance-packages

Repository that hosts packages from the .NET platform whose original home/branch is not building any longer.
MIT License
16 stars 10 forks source link

Microsoft.IO.Redist.File.Exists silently ignores important exception(s) #117

Open MichalPavlik opened 1 month ago

MichalPavlik commented 1 month ago

Hi, I just experienced an issue with File.Exists method in Microsoft.IO.Redist. This method was returning false even when the file exists. The reason was missing assembly redirect for System.Memory in the app.config. A FileLoadException was thrown inside the try/catch block, and silently ignored by https://github.com/dotnet/maintenance-packages/blob/4204ad9a121e7a8ea5cfbb6b46a4d68b3afb2fce/src/Microsoft.IO.Redist/src/System/IO/File.cs#L144

I would propose to not swallow this exception type, so the investigation would be a lot easier :) The fix itself is trivial and I can prepare PR. Just let me know if it would be accepted.

Labeling this as bug, because the method can return false negative result.

ericstj commented 3 weeks ago

The same behavior exists for the actual Exists method in .NET latest https://github.com/dotnet/runtime/blob/13e55a4d617f949fd60e3847841cc70d6bd8e85f/src/libraries/System.Private.CoreLib/src/System/IO/File.cs#L114-L118 And .NETFramework https://referencesource.microsoft.com/#mscorlib/system/io/file.cs,447

propose to not swallow this exception type We can't remove that catch without introducing significant breaking changes.

I can see why this is frustrating though - It's pretty unfortunate that the first place the System.Memory load was hit was when JIT'ing File.Exists. Maybe it would be less breaking to catch and rethrow FileLoadException rather than swallowing like other IOExceptions. @dotnet/area-system-io would need to be convinced to support such a change.

The bug bar for changes to this repo is very high. All packages here are in servicing mode - so we can't really take changes unless they meet the servicing bar.

MichalPavlik commented 3 weeks ago

I totally understand. We also own projects in maintenance mode, and this is an edge case scenario :) I hope this issue will at least help others to quickly investigate why the method returns incorrect value. Feel free to close it as 'won't fix'.