-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #9697: tasks#taskExecutions
not return running tasks after refresh
#10330
Fix #9697: tasks#taskExecutions
not return running tasks after refresh
#10330
Conversation
…ks after refresh Signed-off-by: Esther Perelman <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good, as far as I can tell. However, I can't really test the PR, because for me, when I run a "sleep task" (timeout /T 500
on Window), I never see the task when I invoke "Show running tasks...".
I'm also on Windows, The task in the How to test doesn't work for you? |
That never shows the "sleep" task in master, even in master. What am I missing? |
@tsmaeder Running
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me, and it seems to work well on both Windows and Ubuntu.
@tsmaeder Do you still plan on looking into this? I'll hold back my approval for now.
@msujew code looks good to me and I think on test run is enough for the size of the change.
While the task may be finished, the "cmd" process is still present in the Windows task manager. But I can't imagine this being related to the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, then I'll be approving this, thanks @EstherPerelman 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have verified that this PR in Linux and it works as expected 👍
The code looks good to me as well !
Thanks @EstherPerelman !
Signed-off-by: Esther Perelman [email protected]
The issue: On
TasksExtImpl
constructorthis.fetchTaskExecutions()
was supposed to initialexecutions
map with the running tasks so that when plugins ask fortasks#taskExecutions
theexecutions
map is returned with the running tasks full with the fetched ones,But I found that the
fetchTaskExecutions
function never executes - because at this time (inside the constructor) theRPC
not completely loading the proxy (setting a timeout of several seconds to the execution offetchTaskExecutions
- made it work).The solution: So I wrote this PR in which I removed the call to
fetchTaskExecutions
from theTasksExtImpl
constructor but addedinitTaskLoaded
insideTasksExtImpl
constructor.What it does
Fix #9697
How to test (you can use the same steps to reproduce the issue on master)
task-sample.zip
{ "label": "sleep", "type": "shell", "command": "ping -n 88 127.0.0.1 >nul" }
sleep
taskTerminal
->Show Running Tasks
ctrl+shift+p
->Show Running Tasks **Length**
running:1
need to be shown, Otherwise (the buggy result):running:0
Review checklist
Reminder for reviewers