-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[release/7.0] [MONO][MARSHAL] Initialize ilgen with a flag #83813
[release/7.0] [MONO][MARSHAL] Initialize ilgen with a flag #83813
Conversation
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
Windows ICU failures are known #77485. Other CI failures are explained by issues linked above or are unrelated to this PR. |
Approved by Tactics via email. Will merge once the branches open again. |
Is this fix also applicable to 6.0? We use Mono on 6.0 for s390x platforms, would that benefit from a fix like this too? cc @uweigand |
@omajid If you are experiencing these race conditions with your setup, we can backport the fix to 6.0 also. |
Thanks. I think I have seen them occasionally, but I don't have a great way to reproduce this specific issue on 6.0 (on s390x) as opposed to 7.0 (with ppc64le), where it's super easy for me to run into this. Should we be defensive and backport the change anyway, or would you prefer that I find a way to reproduce the problems with 6.0 specifically? |
I would suggest to create a new issue with repro steps whenever you hit the issue using 6.0. We can validate with the fix and immediately start the backport process. |
I'm retargeting this PR to the new Repo maintainers will now be allowed to merge their own servicing PR as long as it meets the requirements:
The new process is described here: runtime/docs/project/library-servicing.md. The infra team will be actively monitoring servicing PRs to ensure all requirements are met and to help with any issues. Let me know if you have any questions. |
Reminder: April 10th is the last day to merge backport PRs to ensure they get included in the May Release. PR owners are now in charge of merging their own PRs. |
@carlossanlop I'd like to confirm, do I need to do anything else before merging? I've rechecked the CI - the failures are unrelated and explained by the issues linked and we have approval. /cc @SamMonoRT |
[ All good. Smash that squash&merge button, @jandupej. |
Backport of #77448 to release/7.0
/cc @jandupej @naricc
Customer Impact
This resolves a race condition that can occur when ilgen callbacks are being lazily initialized. After the fix, the callbacks are installed only once before any user code is executed, removing the race. The issue has been observed by a customer #83804 on 7.0.
Testing
Manual testing.
Risk
Fairly low. No new features are added. Mono marshaller initialization is merely moved to a location where the said race condition cannot occur.
IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.