EnsureSettingsUpdated returning null for Task

[WardenGnaw]

This is probably not the correct fix, but this solves #1297 issue.

Although we have a null check on line 382, we are still returning null for _updateTask.
All instances of _updateTask are guarded by _updateLock so I am not exactly sure why this is needed.

The issue is we get a setExceptions command and most likely a background thread is going to process FlushSettingsUpdates. At the same time, we get a continue command which fires EnsureSettingsUpdated but a null is returned instead of a Task which gets awaited at

MIEngine/src/MIDebugEngine/Engine.Impl/DebuggedProcess.cs

Line 1674 in 8d70a37

await ExceptionManager.EnsureSettingsUpdated();

[gregg-miskelly]

            if (_updateTask != null)

I also don't see how this is possible. But I would probably instead recomend:

Task updateTask = _updateTask;
if (updateTask != null)
{
    // If we are still delaying our processing, stop delaying it
    _updateDelayCancelSource?.Cancel();

   return updateTask;
}

---

Refers to: src/MIDebugEngine/Engine.Impl/ExceptionManager.cs:382 in b67c1eb. [](commit_id = b67c1eb, deletion_comment = False)

[gregg-miskelly]

                return Task.FromResult<object>(null);

nit: while you are hear, do you want to change this to Task.CompletedTask?

---

Refers to: src/MIDebugEngine/Engine.Impl/ExceptionManager.cs:408 in b67c1eb. [](commit_id = b67c1eb, deletion_comment = False)

[gregg-miskelly]

Do we have a way to reproduce the problem?

[WardenGnaw]

Do we have a way to reproduce the problem?

Yes, but I haven't found a good way to get a debug session to investigate it.

Instructions:

1. docker run -it -cap-add=SYS_PTRACE --security-opt seccomp=unconfined puremourning/vimspector:manual-x86_64 bash
2. In the container, git clone https://github.com/puremourning/vimspector
3. cd /home/dev/vimspector
4. ./run_tests --install breakpoints.test.vim:Test_DisableBreakpointWhileDebugging\(
5. Tests will run and then you will see a failure:

%DONE - built test programs
Running Vimspector Vim tests

%RUN: breakpoints.test.vim:Test_DisableBreakpointWhileDebugging(
%FAIL: breakpoints.test.vim FAILED - see /home/dev/vimspector/test-base/tests/logs/breakpoints.test.vim
Done running tests

1. Look at /home/dev/vimspector/test-base/tests/logs/breakpoints.test.vim/debuglog near the end of the file.

2022-03-26 14:09:32,834 - DEBUG - Sending Message: {"command": "setBreakpoints", "arguments": {"source": {"name": "simple.cpp", "path": "/home/dev/vimspector/tests/testdata/cpp/simple/simple.cpp"}, "breakpoints": [{"line": 16}], "sourceModified": false}, "seq": 18, "type": "request"}
2022-03-26 14:09:32,834 - DEBUG - Sending Message: {"command": "setFunctionBreakpoints", "arguments": {"breakpoints": []}, "seq": 19, "type": "request"}
2022-03-26 14:09:32,835 - DEBUG - Sending Message: {"command": "setExceptionBreakpoints", "arguments": {"filters": []}, "seq": 20, "type": "request"}
2022-03-26 14:09:32,847 - DEBUG - Message received: {'type': 'response', 'request_seq': 18, 'success': True, 'command': 'setBreakpoints', 'body': {'breakpoints': [{'id': 2, 'verified': True, 'line': 16, 'BoundBreakpoints': []}]}, 'seq': 41}
2022-03-26 14:09:32,847 - DEBUG - Message received: {'type': 'event', 'event': 'breakpoint', 'body': {'reason': 'changed', 'breakpoint': {'id': 2, 'verified': True, 'line': 16, 'BoundBreakpoints': []}}, 'seq': 42}
2022-03-26 14:09:32,848 - DEBUG - Message received: {'type': 'response', 'request_seq': 19, 'success': True, 'command': 'setFunctionBreakpoints', 'body': {'breakpoints': []}, 'seq': 43}
2022-03-26 14:09:32,848 - DEBUG - Message received: {'type': 'response', 'request_seq': 20, 'success': True, 'command': 'setExceptionBreakpoints', 'body': {}, 'seq': 44}
2022-03-26 14:09:32,849 - DEBUG - Sending Message: {"command": "continue", "arguments": {"threadId": 416}, "seq": 21, "type": "request"}
2022-03-26 14:09:32,884 - DEBUG - Message received: {'type': 'event', 'event': 'output', 'body': {'category': 'stderr', 'output': "ERROR: Internal error in MIEngine. Exception of type 'System.NullReferenceException' was thrown.\r\n\r\n   at Microsoft.MIDebugEngine.DebuggedProcess.Execute(DebuggedThread thread)\n   at Microsoft.MIDebugEngine.WorkerThread.TrySetOperationInternal(Delegate op)\n   at Microsoft.MIDebugEngine.WorkerThread.SetOperationInternal(Delegate op)\n   at Microsoft.MIDebugEngine.WorkerThread.RunOperation(AsyncOperation op)\n   at Microsoft.MIDebugEngine.AD7Engine.Continue(IDebugThread2 pThread)\n"}, 'seq': 45}
2022-03-26 14:09:32,887 - DEBUG - Message received: {'type': 'response', 'request_seq': 21, 'success': False, 'command': 'continue', 'message': 'Unable to continue. Operation failed with error code 0x80004004.', 'body': {}, 'seq': 46}
2022-03-26 14:09:32,888 - ERROR - Request failed (unhandled): Unable to continue. Operation failed with error code 0x80004004.

I've built MIEngine in WSL, published it and docker cp to the container to get debug info. Attaching a debugger messes up the test suite and we get exceptions in random places.

The validation of this fix was, there was no more System.NullReferenceException in the logs and the continue command worked. The test suit will fail either way.

[WardenGnaw]

I was able to capture a dump but not sure how helpful the stacks are.

This is before _updateTask is null.

[gregg-miskelly]

Assuming this really is somehow because null is being returned from EnsureSettingsUpdated, LGTM.

[gregg-miskelly]

I think I figured out what is going -- I think _updateDelayCancelSource.Cancel(); is causing FlushSettingsUpdates to get to run because it is in await Task.Delay(50, _updateDelayCancelSource.Token);. This then will set _updateTask to null. So your fix looks right.

[WardenGnaw]

Tests are known to be flaky at the moment. Looking into disabling / resolving issues in another PR.