Conversation
g-arjones
force-pushed
the
fix_terminal_process_id
branch
from
f9835ad to
0ce9601
Compare
| this._processManager.onProcessExit(exitCode => this._onProcessExit(exitCode)); | ||
| this._processManager.onProcessData(data => this._onData.fire(data)); | ||
|
|
||
| this._onProcessLaunching.fire(this); |
There was a problem hiding this comment.
I think the ID may not be ready at this time (race condition), this._processManager.onProcessReady should instead be used as the new ID will definitely be set:
There was a problem hiding this comment.
Sorry, I don't get your point. I am not setting nor using the process id here. This onProcessLaunching event is fired just so that the terminal instance can create the promise and store the resolver which can be done as soon as a new process request is received. Or am I missing something?
There was a problem hiding this comment.
In fact, this used to be done in the terminal's constructor...
There was a problem hiding this comment.
+1 to using onProcessReady to ensure that the ID is already set.
There was a problem hiding this comment.
I'm obviously missing the point here. I mean, onProcessLaunching signals that a new process will be launched whereas onProcessIdReady signals that the process DID launch. They serve different purposes in my opinion.
Besides, why have these events nested? If onProcessLaunching is going to be fired by onProcessIdReady isn't it more sensible to just create the process Id promise in onProcessIdReady anyway?
Could you please elaborate on that just for the sake of clarity?
There was a problem hiding this comment.
You're using the event and then passing in instance
https://github.com/Microsoft/vscode/pull/60111/files#diff-738f70b1596a3687ce11b464cd29bfc2R283
That event listener then uses instance.id:
https://github.com/Microsoft/vscode/pull/60111/files#diff-fef4270273da47cf116c1d80fe3da649R38
This is what we're concerned about, instance.id may still be out of date but it wouldn't be if this._processManager.onProcessReady was used instead to fire the original event.
There was a problem hiding this comment.
@Tyriar Hmmm.. But isn't that the terminal instance id (rather than the process id) ? The terminal instance id is set in its constructor here:
And should not depend on the process id being set and is never changed/updated. Is that correct?
alexr00
left a comment
alexr00
left a comment
There was a problem hiding this comment.
+1 to using onProcessReady to ensure that the ID is already set.
|
@Tyriar Could you clarify, please? |
|
Some feedback would be highly appreciated... |
Tyriar
force-pushed
the
fix_terminal_process_id
branch
from
65d98c0 to
e6254db
Compare
Tyriar
left a comment
Tyriar
left a comment
There was a problem hiding this comment.
@g-arjones thanks for the investigation and the great repro, they were very useful to get to the bottom of this. I ended up coming with a simpler fix to just recreate the promiseId using the existing mechanism. And sorry about the delay, I was out all November. 😃
3 participants
@Tyriar AFAICT, the process Id promise resolver is invalidated once the process id is set.
Please let me know if there is a problem with this...
Fix #59635