Use ToolTask.TaskProcessTerminationTimeout correctly

[gpwen]

Use ToolTask.TaskProcessTerminationTimeout as termination time-out to kill external tool when it was cancelled or timed out.

Fixes #8545

Context

The protected property ToolTask.TaskProcessTerminationTimeout has been initialized but never used. Looks like it should be used as timeout on the following line when killing external tool on time-out or cancellation:
https://github.com/dotnet/msbuild/blob/main/src/Utilities/ToolTask.cs#L940

Changes Made

Use ToolTask.TaskProcessTerminationTimeout as termination time-out to kill external tool when it was cancelled or timed out.

Testing

All ToolTask unit test passed.

Notes

No new unit test created.

[JanKrivanek]

Thank you for the contribution!
Looks good!

It would be nice to have this more defensive and explicitly error out on disallowed values (negative, other then -1 - https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.process.waitforexit?view=net-7.0#system-diagnostics-process-waitforexit(system-int32)). Either in the TaskProcessTerminationTimeout setter or in KillToolProcessOnTimeout

[Forgind]

Thank you for the contribution! Looks good!

It would be nice to have this more defensive and explicitly error out on disallowed values (negative, other then -1 - https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.process.waitforexit?view=net-7.0#system-diagnostics-process-waitforexit(system-int32)). Either in the TaskProcessTerminationTimeout setter or in KillToolProcessOnTimeout

I vote setter 🙂 If KillToolProcessOnTimeout fails, I'd assume I'd called that wrong—perhaps the process had already exited, or I didn't have access to it—rather than that the timeout was improperly set.

[Forgind]

Thanks for the change!

[danmoseley]

What is the difference between this and public virtual int Timeout ?

Would it be possible to add a unit test?

[gpwen]

It would be nice to have this more defensive and explicitly error out on disallowed values (negative, other then -1)

Hi @JanKrivanek, you're absolutely right. There must be some checking since that's meant to be used by derived class. However, I'm not sure how to report error. It feels like the correct way is checking that in ValidateParameters() and log build errors. But that would confuse end user (they didn't set anything).

How about exception? Or silently reject invalid setting in setter? Like what's been done with the "MSBUILDTOOLTASKCANCELPROCESSWAITTIMEOUT" environment string. Or silent clipping? What's the official way to do such derived class setting validation in MSBuild tasks?

[gpwen]

What is the difference between this and public virtual int Timeout ?

Timeout is the time-out for external tool execution; TaskProcessTerminationTimeout is the time-out for killing the tool process when it needs to be terminated.

Would it be possible to add a unit test?

I've considered that, but it's well in the process manipulation territory. I'm not even sure if I can create an effective one to verify the effect of that parameter. It would look like some unit tests for Process.KillTree().

[gpwen]

More thoughts about ToolTask.TaskProcessTerminationTimeout validation: Looks like "silent rejection" in setter is the most backward-compatible way, considering the property was originally ignored. That way, derived classes that currently set the parameter to invalid value won't be suddenly broken.

Besides, we have the prior art of the MSBUILDTOOLTASKCANCELPROCESSWAITTIMEOUT environment setting handling.

[Forgind]

More thoughts about ToolTask.TaskProcessTerminationTimeout validation: Looks like "silent rejection" in setter is the most backward-compatible way, considering the property was originally ignored. That way, derived classes that currently set the parameter to invalid value won't be suddenly broken.

Besides, we have the prior art of the MSBUILDTOOLTASKCANCELPROCESSWAITTIMEOUT environment setting handling.

This sounds reasonable, but I do like the idea of an explicit error as JanKrivanek suggested. If we want a compromise, perhaps we could have a user-visible error behind a change wave if they set it to an invalid value? Then we could gather data as to how many people are actually setting it to something invalid; if it's common, we can switch to the silent rejection, and if no one complains, it'll eventually become the default. How does that sound?

[gpwen]

In fact I like the idea of failing early and failing loudly, just a little bit concerned about breaking existing code.

I pushed an update. The setter will now throw ArgumentOutOfRangeException, and unit tests have been added to test the validation logic. Please have a check.

[Forgind]

Often in tasks, we have custom messages with error codes rather than throwing ArgumentOutOfRangeException or the equivalent. It means that if a user hits that exception, they can search for the error code (even if it's in another language, and the rest of the message is confusing) and find relevant documentation. I suspect this particular error won't be hit too much, so I'm ok with leaving it as-is but don't feel strongly.

[JanKrivanek]

@gpwen thank you for this contribution - do you plan to contribute the suggested changes (legacy timeout when opted-out via changewave, and validation within ValidateParameters())?