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

Invocations refactoring #85

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

nerzhulart
Copy link
Contributor

@nerzhulart nerzhulart commented Dec 20, 2016

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. 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
Copy link
Contributor Author

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

operationsToCancel.Remove (methodCall);
ST.Monitor.PulseAll (operationsToCancel);
}
if (wasAborted)
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no Aborted property on AsyncOperation now, Cancellation is controlled by Task API

Copy link
Member

Choose a reason for hiding this comment

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

Ok

if (!op.RawTask.Wait (ShortCancelTimeout)) {
try {
BusyStateChanged (this, new BusyStateEventArgs {IsBusy = true, Description = desc});
op.RawTask.Wait (LongCancelTimeout);
Copy link
Member

Choose a reason for hiding this comment

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

There is an important change in the logic around here. In the old implementation, an invocation abort attempt (InternalAbort) doesn't return until the invocation is aborted or the debug session ends, and keeps trying to abort meanwhile. In this implementation it just tries to cancel once, and returns after a "long" timeout. That's not right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made a lot of experiments and found that many calls of Abort doesn't aborts evaluation if the first call didn't abort successfully. It's right for CorDebug and SBD as well. You may try to run eval of method with loop
void RunLoop() { int i = 0; while (true) { Console.WriteLine(i++); } }
And try to abort it many times, you will fail with timeout always,

Copy link
Member

Choose a reason for hiding this comment

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

Maybe in some cases will always timeout, maybe in others will not. In any case, we can't leave the "Busy" state until the debugger really stopped executing the method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a big issue with your logic (that was used in MD now): if we try to abort some calculation that cannot be aborted gracefully (e.g. endless loop) the debugger may NOT to fire any signal to us - finished or aborted - and in this case we can wait forever. I've already faced with this that's why I've rewritten this to use timeouts. It's absolutely right for CorDebug, I'm not sure about SDB. Also ICorDebugEval has IsActive property, but after any times of Abort() on endless loop or some other long calculation it says us 'true'. I've read sources of .net core (because ICD impl in .net core is pretty the same as full framework) and they just wait 500 ms after Abort call and only after that your the ICD goes to consistent state. So waiting for IsActive == false may lead to forever awaiting.

Copy link
Member

Choose a reason for hiding this comment

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

if we try to abort some calculation that cannot be aborted gracefully (e.g. endless loop) the debugger may NOT to fire any signal to us - finished or aborted - and in this case we can wait forever.

That's is correct, and it is the whole point of having the debugger Busy mode. In SDB, if an invocation is in progress, we can't do anything else with the debugger. So, if the invocation can't be aborted, the only thing we can do is wait forever until the debuggee is able to end the invocation, or until the user stops the debugging session. When we enter the Busy mode, a dialog is shown to the user asking what to do (keep waiting for the call to end or stop the debugging session).

If this is not a problem for CorDebug, then you can add a flag to avoid waiting forever, but we need to keep the current behavior for SDB.

Copy link
Contributor Author

@nerzhulart nerzhulart Jan 17, 2017

Choose a reason for hiding this comment

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

I've restored the inifinite waiting. Now it should work as before. Also I've improved exception handling in SDB invocations. See commit 102fa7a

@@ -0,0 +1,22 @@
namespace Mono.Debugging.Evaluation
{
public interface IAsyncOperation
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right! Thanks

Copy link
Member

Choose a reason for hiding this comment

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

Still unused...


public static class OperationResultEx
{
public static OperationResult<TValue> ThrowIfException<TValue> (this OperationResult<TValue> result, EvaluationContext ctx)
Copy link
Member

Choose a reason for hiding this comment

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

Why not just make it a member of OperationResult?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess this approach makes the code more simple. OperationResult is just a data class without any internal logic

Copy link
Member

Choose a reason for hiding this comment

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

That looks unnecessary to me. In any case, up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case ThrowIfException is more helper method rather than required to be in the class. It's IMHO

} catch (InvocationException ex) {
if (!Aborting && ex.Exception != null) {
string ename = ctx.Adapter.GetValueTypeName (ctx, ex.Exception);
var vref = ctx.Adapter.GetMember (ctx, null, ex.Exception, "Message");
Copy link
Member

Choose a reason for hiding this comment

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

Why is this logic gone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've replaced simple Exception message to full exception object. So now you can walk through it, but before you can see only exception message

Copy link
Member

Choose a reason for hiding this comment

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

How is that object shown to the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an ObjectValue with its children that has IsException flag. It's shown in watchpad as ordinary object

Disposing = true;
lock (operationsToCancel) {
foreach (AsyncOperation op in operationsToCancel) {
op.InternalShutdown ();
Copy link
Member

Choose a reason for hiding this comment

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

All the Shutdown logic is gone, why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shutdown had nearly the same logic as the Cancel. I've unified them into Cancel.

Copy link
Member

Choose a reason for hiding this comment

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

Nearly the same logic, but not the same. Let's say an invocation is made. While the caller is waiting for the invocation to end, the manager is disposed, so the call is canceled. But if the call can't really be cancelled, the caller will wait forever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do you think so? After Dispose is called the Invoke method on another thread will throw EvaluationAbortedException

Copy link
Member

Choose a reason for hiding this comment

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

If the .Abort() call on the invocation fails, or can't be completed (it can happen, that's why there is all that abort retry and debugger busy mode infrastructure), then the invocation will never end, and EvaluationAbortedException will never be thrown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. See below

}
}

string GetInfo ()
Copy link
Member

Choose a reason for hiding this comment

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

More logic that has been removed for no apparent good reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you mean logic of getting exception message so look at line 2230. SoftOperationResult wraps exception object to show it like ordinary object for user

Copy link
Member

Choose a reason for hiding this comment

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

I mean GetInfo(), which is used to log the method and type name when there is an invocation error.

Copy link
Contributor Author

@nerzhulart nerzhulart Jan 10, 2017

Choose a reason for hiding this comment

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

Seems that GetInfo used only for logging. Isnt't it enough to use Description for this? Now I log all this states using this property

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if it is enough or not, it needs to be checked. I just don't want code to be removed if there isn't a good reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main reasons to remove are simplifying and avoiding duplications.

Copy link
Member

Choose a reason for hiding this comment

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

Let's bring it back then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@@ -41,7 +41,7 @@ namespace Mono.Debugging.Evaluation
/// will then be made asynchronous and the Run method will immediately return an ObjectValue
/// with the Evaluating state.
/// </summary>
public class AsyncEvaluationTracker: RemoteFrameObject, IObjectValueUpdater, IDisposable
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were no usages of AsyncEvaluationTracker by this interface.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you used by some dynamic way I can revert this change

Copy link
Member

Choose a reason for hiding this comment

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

The important bit is that RemoteFrameObject is a MarshalByRefObject, and AsyncEvaluationTracker needs to be remotable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you please explain how is this working? where to look at the remoting?

Copy link
Member

Choose a reason for hiding this comment

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

The library is designed so that the debugging engine can run in a remote process, which communicates with the client using remoting. That's really up to the debugger engine implementation. In such scenario, the AsyncEvaluationTracker could be executing in the remote process and generating ObjectValue instances that are serialized into the client process and which may need to hold a reference to that remote object (

return ObjectValue.CreateEvaluating (this, new ObjectPath (id, name), flags);
). This model was necessary with the old Mono debugger, but it is not with the soft debugger. However, the infrastructure is still there, in case a specific debugger implementation needs it.

Copy link
Member

Choose a reason for hiding this comment

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

Please restore the RemoteFrameObject base class.

{
Task RawTask { get; }
string Description { get; }
void AfterCancelled (int elapsedAfterCancelMs);
Copy link
Member

Choose a reason for hiding this comment

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

What is AfterCancelled for? it doesn't seem to have any implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used in CorDebug (monodevelop repo). It's done to ensure that we awaited enough amount of time after Abort() call. Otherwise we can get exceptions in CorDebug.

@nerzhulart
Copy link
Contributor Author

nerzhulart commented Jan 10, 2017

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.,

@slluis
Copy link
Member

slluis commented Jan 10, 2017

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.

@nerzhulart
Copy link
Contributor Author

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

@nerzhulart
Copy link
Contributor Author

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

@nerzhulart nerzhulart force-pushed the invocationsRefactoring branch 2 times, most recently from 0657baa to 9809e72 Compare January 16, 2017 17:38
@nerzhulart
Copy link
Contributor Author

nerzhulart commented Jan 18, 2017

Do you have any new suggestions for this PR?

@nerzhulart
Copy link
Contributor Author

@slluis Hi! Any news on this?

@slluis
Copy link
Member

slluis commented Jan 20, 2017

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

@nerzhulart
Copy link
Contributor Author

Ok! Thanks. Will wait.

@nerzhulart
Copy link
Contributor Author

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

@nerzhulart
Copy link
Contributor Author

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

nerzhulart and others added 11 commits March 16, 2017 20:15
…that wraps target process exception that occurred during runtime call. This allows to inspect exception objects in watch window if eval thrown an exception.
…lows to properly show an exception occurred during property evaluation.
Ignore eval exceptions occured during ToString() calls in type presentation
Better handling of exeptions in Soft invocations
…dateSessionState() is invoked (in finally)

(cherry picked from commit 3befe19)
@nerzhulart
Copy link
Contributor Author

Added commits for Win32 debugger (moved from MD)

@@ -41,7 +41,7 @@ namespace Mono.Debugging.Evaluation
/// will then be made asynchronous and the Run method will immediately return an ObjectValue
/// with the Evaluating state.
/// </summary>
public class AsyncEvaluationTracker: RemoteFrameObject, IObjectValueUpdater, IDisposable
Copy link
Member

Choose a reason for hiding this comment

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

Please restore the RemoteFrameObject base class.

if (!op.RawTask.Wait (ShortCancelTimeout)) {
try {
ChangeBusyState (true, desc);
while (true) {
Copy link
Member

Choose a reason for hiding this comment

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

This loop should gracefully exit if AsyncOperationManager is disposed.

Copy link
Member

Choose a reason for hiding this comment

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

Logic for switching to busy state is now different. Please use old logic.

Copy link
Contributor Author

@nerzhulart nerzhulart Mar 17, 2017

Choose a reason for hiding this comment

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

This loop should gracefully exit if AsyncOperationManager is disposed.

Done

Copy link
Contributor Author

@nerzhulart nerzhulart Mar 17, 2017

Choose a reason for hiding this comment

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

Logic for switching to busy state is now different. Please use old logic.

Can you please explain more detailed?
For me the current logic is quite the same:

  1. StartInvoke
  2. Wait normally for given timeout
  3. If timed out -> Abort()
  4. Wait for Short timeout
  5. If timed out -> Enter busy
  6. Repeat from 4
  7. If awaited -> Exit busy

Seems something like that was before and implemented on magic counters. And the code was very confusing

lock (operationsToCancel) {
foreach (AsyncOperation op in operationsToCancel) {
op.InternalShutdown ();
var desc = op.Description;
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -0,0 +1,22 @@
namespace Mono.Debugging.Evaluation
{
public interface IAsyncOperation
Copy link
Member

Choose a reason for hiding this comment

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

Still unused...

@nerzhulart
Copy link
Contributor Author

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

Fixed

Base automatically changed from master to main March 4, 2021 12:37
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.

2 participants