search

microsoft / vstest

Visual Studio Test Platform is the runner and engine that powers test explorer and vstest.console.
MIT License
870 stars 316 forks source link

make DataCollectorAttachmentProcessorWrapper.ProcessAttachment async #5063

Closed SimonCropp closed 1 month ago

SimonCropp commented 1 month ago

to avoid the 2 Task.Runs and the .GetAwaiter().GetResult()

if there is a reason to sync this async call. let me know and i will do a new PR with a comment to that effect

nohwnd commented 1 month ago

I also don't think the task needs to be offloaded.

@MarcoRossignoli please review.

nohwnd commented 1 month ago

I talked with Marco, and we are not sure why this was done, and we don't want to change it, even though the change looks benign. We are mostly doing maintenance in this repo nowadays, and are focusing on https://github.com/microsoft/testfx/tree/main/src/Platform/Microsoft.Testing.Platform, where we would love your help as well :) Please make the new pr with comment.

MarcoRossignoli commented 1 month ago

Adding some more info about it, the wrapper will run inside a new app domain and Task cannot go out the boundary of appdomain in .NET Framework, plus to adapt the old api/threading we tried to have the same identical "race" (knowing that's hard) to avoid issues like deadlocks that we experienced when we changed the threading model in similar cases around the codebase. Unfortunately VSTest is not fully async and was introduced at best with some hard to fix(without breaking changes) corner case.

The new platform as Jakub reported is fully async from the start.