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

Ensure mono_gc_thread_detach is always called #47816

Merged

Conversation

joncham
Copy link
Contributor

@joncham joncham commented Feb 3, 2021

The previous logic would not call mono_gc_thread_detach for a number of scenarios. One example:

start_wrapper
- mono_thread_info_attach - thread is now live in mono threads layer
- start_wrapper_internal
-- mono_thread_attach_internal - mono attached to vm thread layer. GC handle is set via mono_thread_info_set_internal_thread_gchandle
-- mono_thread_detach_internal - detached from vm thread layer. GC handle cleared via call to mono_thread_info_unset_internal_thread_gchandle
- mono_thread_info_exit
-- mono_thread_info_detach
--- unregister_thread
---- thread_detach callback - checks if gc handle is valid and returns if not via mono_thread_info_try_get_internal_thread_gchandle. We've already cleared above so we never call mono_gc_thread_detach.

This change ensures mono_gc_thread_detach is always called even the GC handle for the thread has already been cleared.

Copy link
Member

@lateralusX lateralusX left a comment

Choose a reason for hiding this comment

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

I assume this is needed on other GC's than sgen that implements logic in mono_gc_thread_detach, sgen's implementation of that function is empty. I can see that Boehm GC implementation does, GC_unregister_my_thread as part of its implementation of mono_gc_thread_detach, so there it will have effects. Might be worth keeping in mind that all CI runs using sgen will then probably not detect any side effects due to this change, but I assume you run plenty on your end using Boehm GC.

@lambdageek
Copy link
Member

/azp run sync-runtime-to-mono

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@joncham
Copy link
Contributor Author

joncham commented Feb 4, 2021

I assume this is needed on other GC's than sgen that implements logic in mono_gc_thread_detach, sgen's implementation of that function is empty. I can see that Boehm GC implementation does, GC_unregister_my_thread as part of its implementation of mono_gc_thread_detach, so there it will have effects. Might be worth keeping in mind that all CI runs using sgen will then probably not detect any side effects due to this change, but I assume you run plenty on your end using Boehm GC.

Yes, this is an issue I encountered in Boehm as part of our upgrading Mono to more recent version. We handle all testing for Boehm usage on our side.

@akoeplinger
Copy link
Member

Android issue is #47850.

@akoeplinger akoeplinger merged commit 37f1fb9 into dotnet:master Feb 5, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants