Open

Code Quality: Introduced a wrapper for non-blocking await on a sync method #17100

Show file tree
Hide file tree
Changes from all commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Failed to load files.
Original file line numberDiff line numberDiff line change
Expand Up@@ -225,3 +225,8 @@ QITIPF_FLAGS
GetKeyboardState
MapVirtualKey
GetKeyboardLayout
CreateEvent
SetEvent
CoWaitForMultipleObjects
CWMO_FLAGS
INFINITE
Original file line numberDiff line numberDiff line change
Expand Up@@ -2,6 +2,9 @@
// Licensed under the MIT License.

using Windows.Win32;
using Windows.Win32.Foundation;
using Windows.Win32.System.Com;
using Windows.Win32.Security;

namespace Files.App.Storage
{
Expand DownExpand Up@@ -143,5 +146,46 @@ public static Task Run(Func<Task> func)

return tcs.Task;
}

public unsafe static Task RunAsSync(Action action)
{
Debug.Assert(Thread.CurrentThread.GetApartmentState() is ApartmentState.STA);

HANDLE hEventHandle = PInvoke.CreateEvent((SECURITY_ATTRIBUTES*)null, true, false, default);

var tcs = new TaskCompletionSource();

Task.Run(() =>
{
try
{
action();
tcs.SetResult();
}
catch (Exception ex)
{
tcs.SetException(ex);
}
finally
{
PInvoke.SetEvent(hEventHandle);
}
});

HANDLE* pEventHandles = stackalloc HANDLE[1];
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you even need to allocate a stack here? I thought your HANDLE variable is in stack already.

Copy link
Member Author

Choose a reason for hiding this comment

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

Related to your 2nd comment, it looks like you can't take address of a local variable that goes thru an async lambda/function. The current code doesn't have async and initially did so this can be refactored but IIUC if we wanna use async void we may have to do this.

pEventHandles[0] = hEventHandle;
uint dwIndex = 0u;

PInvoke.CoWaitForMultipleObjects(
(uint)CWMO_FLAGS.CWMO_DEFAULT,
PInvoke.INFINITE,
1u,
pEventHandles,
&dwIndex);

PInvoke.CloseHandle(hEventHandle);

return tcs.Task;
}
}
}
Original file line numberDiff line numberDiff line change
Expand Up@@ -9,7 +9,6 @@
using System.Text;
using Windows.ApplicationModel.Activation;
using Windows.Storage;
using static Files.App.Helpers.Win32PInvoke;

namespace Files.App
{
Expand All@@ -21,9 +20,6 @@ namespace Files.App
/// </remarks>
internal sealed class Program
{
private const uint CWMO_DEFAULT = 0;
private const uint INFINITE = 0xFFFFFFFF;

public static Semaphore? Pool { get; set; }

static Program()
Expand DownExpand Up@@ -250,20 +246,10 @@ private static async void OnActivated(object? sender, AppActivationArguments arg
/// </remarks>
public static void RedirectActivationTo(AppInstance keyInstance, AppActivationArguments args)
{
IntPtr eventHandle = CreateEvent(IntPtr.Zero, true, false, null);

Task.Run(() =>
STATask.RunAsSync(() =>
{
keyInstance.RedirectActivationToAsync(args).AsTask().Wait();
SetEvent(eventHandle);
});

_ = CoWaitForMultipleObjects(
CWMO_DEFAULT,
INFINITE,
1,
[eventHandle],
out uint handleIndex);
}

public static void OpenShellCommandInExplorer(string shellCommand, int pid)
Expand All@@ -273,20 +259,10 @@ public static void OpenShellCommandInExplorer(string shellCommand, int pid)

public static void OpenFileFromTile(string filePath)
{
IntPtr eventHandle = CreateEvent(IntPtr.Zero, true, false, null);

Task.Run(() =>
STATask.RunAsSync(() =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why async void isn't desirable here?

Copy link
Member Author

Choose a reason for hiding this comment

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

So you mean adding Wait() in that helper method instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean making it an async lambda and then await.

Copy link
Member Author

Choose a reason for hiding this comment

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

Id like to keep this PR's scope just wrapping the unsafe code into a separate method.

{
LaunchHelper.LaunchAppAsync(filePath, null, null).Wait();
SetEvent(eventHandle);
});

_ = CoWaitForMultipleObjects(
CWMO_DEFAULT,
INFINITE,
1,
[eventHandle],
out uint handleIndex);
}
}
}
Loading