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 #59635 #60111

Merged
merged 3 commits into from Dec 13, 2018

Conversation

Projects
None yet
3 participants
@g-arjones
Copy link
Contributor

g-arjones commented Oct 7, 2018

@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

@g-arjones g-arjones force-pushed the g-arjones:fix_terminal_process_id branch from f9835ad to 0ce9601 Oct 9, 2018

@@ -772,6 +774,8 @@ export class TerminalInstance implements ITerminalInstance {
this._processManager.onProcessExit(exitCode => this._onProcessExit(exitCode));
this._processManager.onProcessData(data => this._onData.fire(data));

this._onProcessLaunching.fire(this);

This comment has been minimized.

@Tyriar

Tyriar Oct 11, 2018

Member

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:

this._process.onProcessIdReady(pid => {
this.shellProcessId = pid;

This comment has been minimized.

@g-arjones

g-arjones Oct 11, 2018

Author Contributor

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?

This comment has been minimized.

@g-arjones

g-arjones Oct 11, 2018

Author Contributor

In fact, this used to be done in the terminal's constructor...

This comment has been minimized.

@g-arjones

g-arjones Oct 11, 2018

Author Contributor

/cc @Tyriar

This comment has been minimized.

@alexr00

alexr00 Oct 12, 2018

Member

+1 to using onProcessReady to ensure that the ID is already set.

This comment has been minimized.

@g-arjones

g-arjones Oct 12, 2018

Author Contributor

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?

This comment has been minimized.

@Tyriar

Tyriar Oct 13, 2018

Member

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.

This comment has been minimized.

@g-arjones

g-arjones Oct 15, 2018

Author Contributor

@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:

this._id = TerminalInstance._idCounter++;

And should not depend on the process id being set and is never changed/updated. Is that correct?

@Tyriar Tyriar requested a review from alexr00 Oct 11, 2018

@Tyriar Tyriar added this to the October 2018 milestone Oct 11, 2018

@alexr00
Copy link
Member

alexr00 left a comment

+1 to using onProcessReady to ensure that the ID is already set.

@Tyriar Tyriar modified the milestones: October 2018, Backlog Oct 25, 2018

@g-arjones

This comment has been minimized.

Copy link
Contributor Author

g-arjones commented Oct 26, 2018

@Tyriar Could you clarify, please?

@g-arjones

This comment has been minimized.

Copy link
Contributor Author

g-arjones commented Oct 26, 2018

Some feedback would be highly appreciated...

@Tyriar Tyriar force-pushed the g-arjones:fix_terminal_process_id branch from 65d98c0 to e6254db Dec 13, 2018

@Tyriar Tyriar modified the milestones: Backlog, December 2018 Dec 13, 2018

@Tyriar

Tyriar approved these changes Dec 13, 2018

Copy link
Member

Tyriar left a 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. 😃

Fixed

@Tyriar Tyriar merged commit bf15510 into Microsoft:master Dec 13, 2018

1 of 2 checks passed

VS Code in progress
Details
license/cla All CLA requirements met.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment