PaulHigin / PSThreadJob

A PowerShell module for running concurrent jobs based on threads rather than processes
MIT License
180 stars 18 forks source link

Scriptblock exceptions are not returned #2

Closed ajansveld closed 6 years ago

ajansveld commented 6 years ago

The following yields no output: Start-ThreadJob {throw "foo"} | Receive-Job -Wait

If you use Start-Job instead the exception is returned as a non-terminating error. Is there any way Start-ThreadJob could do the same?

PaulHigin commented 6 years ago

@ajansveld Thanks for pointing this out! The reason this terminating error isn't reported is because, unfortunately, PowerShell doesn't currently provide a public or protected accessible way to report it as part of the job state change. I was aware of this when writing the code but neglected to include a TODO comment and mention this in the release notes. https://github.com/PaulHigin/PSThreadJob/blob/master/ThreadJob/ThreadJob/PSThreadJob.cs#L207

The only way I can see to fix this is to modify PowerShell API to provide protected class access to setting job state along with an optional exception (which would be the terminating exception). This is currently provided as internal access but derived classes such as ThreadJob cannot access the API.

I am sure we can get this change into PSCore (https://github.com/powershell/powershell) but then we would have compatibility issues with Windows PowerShell. But I guess we can have different versions of ThreadJob targeted to different versions of PowerShell.

There is another problem similar to this that I did mention in the release notes. And that is support for processing a $using: variable is also internal access to PowerShell, and so ThreadJob cannot support the using variable without first modifying PowerShell. I'll create an issue for this as well.

ajansveld commented 6 years ago

Hmmm, any chance that inheriting from Job2 in combination with this would do the trick?

case PSInvocationState.Failed:
    SetJobState(JobState.Failed, psStateChanged.InvocationStateInfo.Reason);
    break;
PaulHigin commented 6 years ago

That might work. Job2 has more complex functions that don't ThreadJobs don't need, but maybe they can be NOP'd. But that might play havoc with PowerShell that expect Job2 jobs to behave in a specific way.

ajansveld commented 6 years ago

Understood - perhaps something like this would be simpler then? Probably needs some more error handling (har)...

case PSInvocationState.Failed:
    IContainsErrorRecord reason = psStateChanged.InvocationStateInfo.Reason as IContainsErrorRecord;
    this.Error.Add(reason.ErrorRecord);
    SetJobState(JobState.Failed);
    break;
PaulHigin commented 6 years ago

This would be an adequate workaround. But PowerShell handles job terminating errors differently from non-terminating errors (which appear in the Error collection), and I would like to keep that behavior.

I think the best solution is your first suggestion and derive from Job2. This would mean implementating some of the new APIs (such as start/stop async) so that basic job handling still works. But I think this can be done.

PaulHigin commented 6 years ago

I created a PR to derive from Job2 and handle terminating errors. It seems to work fine and so far I haven't found any weird behavior. https://github.com/PaulHigin/PSThreadJob/pull/6

ajansveld commented 6 years ago

I get an error running this, do you see the same?

Start-ThreadJob {"Ok"} | Receive-Job -Wait -AutoRemoveJob
PaulHigin commented 6 years ago

Yep, I see the same thing. Looks like a problem with Job2 behavior. I originally chose Job1 since its simplest and ThreadJob didn't need to support any Job2 behavior. But I'll have to look at it more closely.

ajansveld commented 6 years ago

FWIW, I am starting to like the second option more (deriving from Job1 and adding the exception as an error). It keeps things simple and IMHO actually improves on the Start-Job behavior:

PaulHigin commented 6 years ago

Well, it is certainly an easier solution. I haven't had time to debug the job2 derivation problems yet. The only downside is that it is not immediately apparent what the job terminating error is. OTOH it should be the last error in the error stream so that may be a good enough indicator.

ajansveld commented 6 years ago

Hi Paul, just curious if this project is still alive - haven't seen any activity in the last few months. Just need to know if I should keep using this or look at alternatives.

Thanks!

PaulHigin commented 6 years ago

I have been pretty busy and haven't had time to work on this lately. However, feel free to make changes and submit PRs. My intention was to let others make changes and write tests.

PaulHigin commented 6 years ago

@ajansveld I found some time and implemented Using keyword and fixed the terminating error by deriving from Job2: https://github.com/PaulHigin/PSThreadJob/pull/12 Let me know if you see an issues.

PaulHigin commented 6 years ago

I was able to fix this by using Job2 and implementing JobSourceAdapter.