More robust solution to execute queueing #8651
Conversation
…oft/vscode-jupyter into rchiodo/qt5_delayedFuture
Codecov Report
@@ Coverage Diff @@
## main #8651 +/- ##
======================================
- Coverage 71% 71% -1%
======================================
Files 382 384 +2
Lines 24523 24624 +101
Branches 3767 3789 +22
======================================
+ Hits 17549 17611 +62
- Misses 5449 5477 +28
- Partials 1525 1536 +11
|
|
A restart test seems to be failing on all versions. Not sure why. I'm investigating now (hopefully it repros locally) |
| @@ -217,7 +219,8 @@ export abstract class BaseJupyterSession implements IJupyterSession { | |||
| if (!this.session?.kernel) { | |||
| throw new SessionDisposedError(); | |||
| } | |||
| return this.session.kernel.requestExecute(content, disposeOnDone, metadata); | |||
| // Make sure to chain the executes | |||
| return this.chainingExecute.requestExecute(this.session.kernel, content, disposeOnDone, metadata); | |||
There was a problem hiding this comment.
Question (not sure myself). If this QT event loop can't handle the queuing would we also need this chain on the other request functions here? Like requestDebug and requestInspect?
There was a problem hiding this comment.
Not sure. It might, but we don't send requestDebug/requestInspect in multiple spots. And requestInspect at the same time as requestExecute does work with QT5. At least I couldn't break it with the variables tab open.
| private metadata?: JSONObject | ||
| ) { | ||
| // Setup our request based on the previous link finishing | ||
| if (previousLink) { |
There was a problem hiding this comment.
This is minor, but was wondering on the typing here a tiny bit. Aren't we guaranteed a previousLink at this point?
There was a problem hiding this comment.
Oh yeah that's true. I can skip the if check.
| // If the kernel dies, finish our future | ||
| this.statusChangedHandler = (_session: Kernel.IKernelConnection, status: KernelMessage.Status) => { | ||
| if (status === 'unknown' || status === 'restarting' || status === 'dead') { | ||
| this.doneDeferred.reject(new CancellationError()); |
There was a problem hiding this comment.
Is this a cancellation error? Also if we are looking at 'restarting' we probably need 'autorestarting' as well since Jupyter has both.
There was a problem hiding this comment.
Yeah I should add 'autorestarting'.
CancellationError is what Jupyter throws on the 'done' promise when the kernel is shutdown
| return this.requestFuture.msg; | ||
| } | ||
| return { | ||
| content: this.content, |
There was a problem hiding this comment.
Just to check my understanding, this path (where the requestFuture is not define) is basically just a dummy placeholder until the previous execute finished right?
There was a problem hiding this comment.
Yes that's correct. I don't think any of our code ever reads 'msg' anyway.
There was a problem hiding this comment.
Sounds good, the values for it just didn't quite make sense, so just wanted to confirm that it was a dummy value (essentially).
For #8322
This implements the 'delayed' future so that it's impossible to send more than one requestExecute at a time.
Tested locally and it works but doing this separately so that it can be reverted if it causes something to break.