Fix some async hangs in the functional tests#5007
Conversation
| service.onRequest(LiveShareCommands.getSysInfo, (_args: any[], cancellation: CancellationToken) => this.onGetSysInfoRequest(cancellation)); | ||
| service.onRequest(LiveShareCommands.restart, (_args: any[], cancellation: CancellationToken) => this.onRestartRequest(cancellation)); | ||
| service.onRequest(LiveShareCommands.interrupt, (args: any[], cancellation: CancellationToken) => this.onInterruptRequest(args.length > 0 ? args[0] as number : LiveShare.InterruptDefaultTimeout, cancellation)); | ||
| service.onRequest(LiveShareCommands.disposeServer, (_args: any[], _cancellation: CancellationToken) => this.dispose()); |
There was a problem hiding this comment.
onRequest [](start = 24, length = 9)
Yay, test actually found a legitimate bug. #Resolved
| out: output, | ||
| dispose: () => { | ||
| if (proc && !proc.killed) { | ||
| tk(proc.pid); |
There was a problem hiding this comment.
tk [](start = 20, length = 2)
This causes a hang on windows because tk uses 'exec' instead of 'execSync' to kill a process. #Resolved
| "webpack-fix-default-import-plugin": "^1.0.3", | ||
| "webpack-merge": "^4.1.4", | ||
| "webpack-node-externals": "^1.7.2", | ||
| "why-is-node-running": "^2.0.3", |
There was a problem hiding this comment.
love the name. #Resolved
| tk(pid); | ||
| if (process.platform === 'win32') { | ||
| // Windows doesn't support SIGTERM, so execute taskkill to kill the process | ||
| execSync(`taskkill /pid ${pid} /T /F`); |
There was a problem hiding this comment.
taskkill is available on all windows systems that we care about? I didn't see docs that specified that off hand, looks like it has been around a while though. #Resolved
There was a problem hiding this comment.
This is what the 'tree-kill' function was doing under the covers. So it would have not worked before either. #Resolved
| } | ||
| if (this.session) { | ||
| if (this.session && !this.session.isDisposed) { | ||
| this.session.dispose(); |
There was a problem hiding this comment.
Shouldn't the dispose functions already be checking the isDisposed flag? #Resolved
There was a problem hiding this comment.
Not sure. Not my code, so just wanted to be sure. #Resolved
Attempt to workaround jupyter issue
| await sleep(0); | ||
| // This function seems to cause CI builds to timeout randomly on | ||
| // different tests. Waiting for status to go idle doesn't seem to work and | ||
| // in the past, waiting on the ready promise doesn't work either. Check status with a maximum of 5 seconds |
There was a problem hiding this comment.
aximum of 5 seconds [](start = 99, length = 19)
Looks like you changed the time.
There was a problem hiding this comment.
Yeah made it 10 seconds. Seems long enough? What do you think?
In reply to: 270593590 [](ancestors = 270593590)
There was a problem hiding this comment.
10 is fine just saying to update the comment.
In reply to: 270593664 [](ancestors = 270593664,270593590)
There was a problem hiding this comment.
Okay will do if there's another change.
In reply to: 270593752 [](ancestors = 270593752,270593664,270593590)
There was a problem hiding this comment.
LGTM, not sure if code flow reported it, but I flipped to reviewing then back to approved.
In reply to: 270593897 [](ancestors = 270593897,270593752,270593664,270593590)
| } | ||
|
|
||
| // If we don't have a kernel spec yet, check using our current connection | ||
| if (!kernelSpec && connection.localLaunch) { |
There was a problem hiding this comment.
localLaunch [](start = 38, length = 11)
Also fixed this bug with remote getting kernel specs when it doesn't need to.
For #4992
Two root causes
After this they should run without hanging, so removing the '--exit' from mocha options
package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed)