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

Catch ArgumentException as well as BadImageFormatException when failing because of libraries without resources #6546

Merged
merged 3 commits into from
Jun 29, 2021

Conversation

Forgind
Copy link
Member

@Forgind Forgind commented Jun 10, 2021

This was a heinous bug to repro.

Using VS 17.0.0+, create a blank UWP app with target platform version 16299 or 17134. Build Debug/x86.
If that doesn't work, clean and build again.
If that doesn't work, clean and build again.

It should work every time, but from my testing, every other clean-build cycle succeeds even following identical steps. Every other build clean-build cycle fails with a message saying an attempt was made to load a file with incorrect format. This is normally a BadImageFormatException and caught (see comment above), but it seems that with dev 17, that is sometimes (always?) wrapped in an ArgumentException and not caught. This fixes that problem by catching both.

Maybe we should verify that the ArgumentException mentions BadImageFormatException before just continuing?

Testing: Several attempted repros in a row succeeded. It had been pretty reliably failing every other time, so that was a shift.

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

I'm concerned that this isn't the full root cause. What is different in dev17? Have you falsified the caching hypothesis I proposed offline?

catch (Exception e)
catch (ArgumentException)
{
// BadImageFormatExceptions can be wrapped in ArgumentExceptions, so catch those, too.
Copy link
Member

Choose a reason for hiding this comment

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

Is there something where we can test an InnerException to validate that this is really the case?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Incorporated, thanks!

@Forgind
Copy link
Member Author

Forgind commented Jun 11, 2021

The repro seems complicated by every other clean-build cycle succeeds even following identical steps. Not sure what could cause that—marcpopMSFT suggested maybe creating a cache then invalidating it on subsequent builds, but if you create a cache, you should use it in the same step, and if you invalidate a cache, you shouldn't use that cache.

That the error message explicitly says it's a BadImageFormatException (even though it's an ArgumentException) and that we were previously catching BadImageFormatExceptions makes me fairly confident it's just a slight change in what error is being thrown. Furthermore, I tried cleaning and building several times with the blanket catch (ArgumentException) and the more specialized one I now have, and in both cases, the rate of failure went from 50% to 0% (in my tests), which suggests the same.

@rainersigwald rainersigwald added this to the 17.0 milestone Jun 14, 2021
Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

Let's go ahead with this since we (sorta) said we'd get it in for preview2.

(I still want to know what's up with the intermittent failure/toggle behavior, but this change looks good independent of that.)

@AR-May AR-May merged commit 1b7661f into dotnet:main Jun 29, 2021
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.

4 participants