dahall / TaskScheduler

Provides a .NET wrapper for the Windows Task Scheduler. It aggregates the multiple versions, provides an editor and allows for localization.
MIT License
1.22k stars 194 forks source link

COMException if TaskService.AddTask called in a different thread from the one where the instance is created #884

Closed Ebola-Chan-bot closed 3 years ago

Ebola-Chan-bot commented 3 years ago

Describe the bug COMException if TaskService.AddTask called in a different thread from the one where the instance is created.

To Reproduce Steps to reproduce the behavior:

  1. Create a VB.Net 5 WPF project in Visual Studio
  2. Add a Sub New() in Class Application in Application.xaml.vb:
    Sub New()
    Dim ts As TaskService = TaskService.Instance
    Threading.Tasks.Task.Run(Sub() ts.AddTask("Test", New DailyTrigger(1), New ExecAction(Process.GetCurrentProcess.MainModule.FileName)))
    End Sub
  3. Debug the project.
  4. System.Runtime.InteropServices.COMException is thrown.

Environment (please complete the following information):

PS: In a Console Application, this exception is not thrown, but the task is not successfully added if you inspect the Windows Task Scheduler.

dahall commented 3 years ago

The underlying COM object from Microsoft that this project uses (ITaskService) is single threaded (STA). Each thread must have it's own instance of TaskService. To make your code above work:

Threading.Tasks.Task.Run(Sub() TaskService.Instance.AddTask("Test", New DailyTrigger(1), New ExecAction(Process.GetCurrentProcess.MainModule.FileName)))

If you have a recommendation for how to handle the exception better, please comment.

Ebola-Chan-bot commented 3 years ago

As you wrote at Classes-OverView:

connecting and disconnecting is an expensive operation

It's certainly expensive to have each thread get its own instance. However, I found a workaround:

Sub New()
    Threading.Tasks.Task.Run(Sub() SetTask("Task1"))
    Threading.Tasks.Task.Run(Sub() SetTask("Task2"))
    Threading.Tasks.Task.Run(Sub() SetTask("Task3"))
End Sub
Sub SetTask(Name As String)
    Static ts As TaskService = TaskService.Instance
    ts.AddTask(Name, New DailyTrigger(1), New ExecAction(Process.GetCurrentProcess.MainModule.FileName))
End Sub

A local static instance is successfully shared among threads. I have not tested about efficiency, but it looks much more comfortable, as the instance seems to be reused. If you have reasons not to adopt this workaround, I suggest that you at least talk about this issue in the documentation. 😀 By the way, I suggest that you define TaskService.Instance as a function instead of a property. A property is often considered as some stored data that won't be expensive to get. However, I read your code and found this property is quite expensive! It's better to use a function to return an instance here, implying that getting this instance is an expensive call so that the caller should try to reuse it, instead of calling this property from time to time. If I want my caller to realize a call is expensive, I'll define it as not only a function, but also an async function that returns a Task(Of Something), strongly implying that this call is very expensive, so you should try your best to reuse it and avoid frequent calls. 😄

dahall commented 3 years ago

TaskService.Instance already is doing exactly as you've done in your code. It is a static property marked with the ThreadStaticAttribute so that it is only created once per thread. I would still recommend the code I posted so that you're not creating another static variable. TaskService.Instance has been tested widely.