[debugger-agent] Fix mono_debugger_disconnect implementation #1217
Conversation
- Fix mono_debugger_disconnect method so it disconnects the debugger client instead of trying to stop the debugger thread.
| @@ -1091,12 +1091,18 @@ mono_debugger_get_generate_debug_info () | |||
| return disable_optimizations; | |||
| } | |||
|
|
|||
| // Declare transport_close2 method | |||
| static void transport_close2(void); | |||
|
|
|||
| MONO_API void | |||
| mono_debugger_disconnect () | |||
There was a problem hiding this comment.
Is this a new function we added or are we changing existing behaviour here?
Would it make sense to put the transport_close2 call into a new function like? mono_debugger_disconnect_external() ?
There was a problem hiding this comment.
transport_close2 is an existing function. In this PR I'm just modifying the implementation of mono_debugger_disconnect.
In my opinion I don't think that mono_debugger_disconnect_external is needed since the mono_debugger_disconnect method is just one call to an existing method.
There was a problem hiding this comment.
I'm asking if you originally added mono_debugger_disconnect or if it already existed and this change could be breaking change.
There was a problem hiding this comment.
mono_debugger_disconnect was added by Jonathan Chambers with the other new APIs needed for scripting/no-cost-editor-attaching
There was a problem hiding this comment.
Okay, great :-) Then no existing functionality will break.
- Move method declaration to top of the file
|
Should we apply the same changes to the debugger code in IL2CPP? |
|
This workflow is just for the Unity Editor so I don't think is necessary in IL2CPP. But honestly I don't know what is the usual workflow you guys do, in case you try to maintain full feature parity or not. Feel free to reach me out in slack if you want to discuss more about it |
Purpose of this PR:
Current mono_debugger_disconnect implementation is trying to stop the debugger thread and start it again, this approach is not working as expected since it creates a deadlock.
Since the debugger the thread is blocked in the RECV method the new approach is just to shutdown the debugger client connection socket so the client disconnects and the debugger_thread loop already handles the debugger client disconnection properly.
Testing status:
Florence has tested the disconnection feature in the
scripting/no-cost-editor-attaching-disconnect-fixbranch.