cherry pick CLIWrapper deadlock fix#14662
Conversation
| var completedTask = Task.WhenAny(readStdOutTask, Task.Delay(TimeSpan.FromMilliseconds(timeoutms))).Result; | ||
| //if the completed task was our read std out task, then return the data | ||
| //else we timed out, so return an empty string. | ||
| return completedTask == readStdOutTask ? readStdOutTask.Result : string.Empty; |
There was a problem hiding this comment.
we might be able to simplify lines 148 to 151 by using
return readStdOutTask.Wait(timeoutms) ? readStdOutTask.Result : string.Empty;
There was a problem hiding this comment.
either way we go, please test that the timeout can be reached without issues.
Ex Add a Thread.Sleep(timeoutms *2) in the TaskRun delegate.
There was a problem hiding this comment.
@pinzart90 - does this test already cover what you are concerned about - or not?
https://github.com/DynamoDS/Dynamo/blob/master/test/Libraries/DynamoUtilitiesTests/CLIWrapperTests.cs#L47
There was a problem hiding this comment.
@pinzart90 I've added another test and slightly modified the GetData method to make it possible to inject a mock readLine delegate that will be invoked inside the Task delegate so we can sleep or return data etc.
|
I am going to merge this.
|
* Tibi's changes * review comments * fix test
4 participants
Purpose
cherry pick fixes from #14649 for CLIWrapper classes
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
(FILL ME IN) Brief description of the fix / enhancement. Mandatory section
Reviewers
(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)
(FILL ME IN, optional) Any additional notes to reviewers or testers.
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of