debugger-libs icon indicating copy to clipboard operation
debugger-libs copied to clipboard

Published 20 hours ago • verified publisher icon mono

Invocations refactoring

Open nerzhulart opened this issue 8 years ago • 13 comments

This refactoring originally was started to fix multiple deadlocks in AsyncOperationManager. After some investigation I've decided to rewrite all of this code to .net Task API to avoid syncronization issues caused by bad desing and wrong using of ManualResetEvents everywhere. The main changes:

  • I'ive introduced a new EvaluatorExceptionThrownException that wraps target exception object and allows to inspect this object in watch pad when your eval has thrown an exception.
  • AsyncOperationManager.Invoke is explicitly synchronous now and returns OperationResult<T>. Also it was synchronous before as well, but returned void and the call site had to obtain the result in other field and it was complicated.
  • I've added a lot of logging into this code

After this PR merged I'm going to add the second counterpart PR in monodevelop repo to supply these changes for CorDebug

nerzhulart avatar Dec 20 '16 16:12 nerzhulart

Hey, guys! Any updates on this? Tests on windows go fine.

nerzhulart avatar Dec 30 '16 22:12 nerzhulart

I have to say that this code works in Rider since November and the evaluation experience was greatly improved as well as performance. Before this we faced with deadlocks many times per day. Also we didn't notice any evaluation issues after we merged this change into our production code.,

nerzhulart avatar Jan 10 '17 15:01 nerzhulart

Even if it works for Rider, I need to make sure that it doesn't cause any regression in XS. It is an important change in a critical piece of code, so we need to be very careful about it.

slluis avatar Jan 10 '17 17:01 slluis

Yes, of course! I just wanted to say that this code was not written a day ago and pushed without any testing

nerzhulart avatar Jan 10 '17 17:01 nerzhulart

I've removed IAsyncOperation.cs and restore AsyncOperationTracker to be RemoteFrameObject.

nerzhulart avatar Jan 10 '17 17:01 nerzhulart

Do you have any new suggestions for this PR?

nerzhulart avatar Jan 18 '17 13:01 nerzhulart

@slluis Hi! Any news on this?

nerzhulart avatar Jan 20 '17 16:01 nerzhulart

Sorry, it has been a busy week. I'll review it asap.

slluis avatar Jan 20 '17 16:01 slluis

Ok! Thanks. Will wait.

nerzhulart avatar Jan 20 '17 16:01 nerzhulart

Added a couple of commits. Now the Abort() is being called in loop as it was before.

nerzhulart avatar Jan 23 '17 19:01 nerzhulart

Hi! Any news on this? I have new fixes for value modifications, but they are dependent on this

nerzhulart avatar Mar 16 '17 12:03 nerzhulart

Added commits for Win32 debugger (moved from MD)

nerzhulart avatar Mar 16 '17 19:03 nerzhulart

It doesn't take into account that the method may already be being aborted, like the old implementation did.

Fixed

nerzhulart avatar Mar 17 '17 15:03 nerzhulart