-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
Don't connect LocalTerminalBackend until LifecyclePhase.Restored #182631
Conversation
private readonly _ptys: Map<number, LocalPty> = new Map(); | ||
// HACK: ProxyChannels behave strangely when interacting with promises and seemingly never | ||
// resolve, so _proxyReady is awaited before every proxy use | ||
private _proxy?: IPtyService; |
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.
We should probably follow up on this, I am curious what the issue is.
// HACK: ProxyChannels behave strangely when interacting with promises and seemingly never | ||
// resolve, so _proxyReady is awaited before every proxy use | ||
private _proxy?: IPtyService; | ||
private readonly _proxyReady: DeferredPromise<void>; |
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.
This could return the proxy so that you do not have to do all the proxy!
calls.
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.
That's what I did initially, the promise resolve function was definitely called, but .p
was always pending. This happened with a regular promise as well. It would be nice to figure out what exactly was going on but I wasted quite a lot of time on it already
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.
Shared process is not using a deferred promise but just this if you want to copy:
vscode/src/vs/workbench/services/sharedProcess/electron-sandbox/sharedProcessService.ts
Lines 19 to 52 in 7a7d457
private readonly withSharedProcessConnection: Promise<MessagePortClient>; | |
private readonly restoredBarrier = new Barrier(); | |
constructor( | |
readonly windowId: number, | |
@ILogService private readonly logService: ILogService | |
) { | |
super(); | |
this.withSharedProcessConnection = this.connect(); | |
} | |
private async connect(): Promise<MessagePortClient> { | |
this.logService.trace('Renderer->SharedProcess#connect'); | |
// Our performance tests show that a connection to the shared | |
// process can have significant overhead to the startup time | |
// of the window because the shared process could be created | |
// as a result. As such, make sure we await the `Restored` | |
// phase before making a connection attempt, but also add a | |
// timeout to be safe against possible deadlocks. | |
await Promise.race([this.restoredBarrier.wait(), timeout(2000)]); | |
// Acquire a message port connected to the shared process | |
mark('code/willConnectSharedProcess'); | |
this.logService.trace('Renderer->SharedProcess#connect: before acquirePort'); | |
const port = await acquirePort('vscode:createSharedProcessMessageChannel', 'vscode:createSharedProcessMessageChannelResult'); | |
mark('code/didConnectSharedProcess'); | |
this.logService.trace('Renderer->SharedProcess#connect: connection established'); | |
return this._register(new MessagePortClient(port, `window:${this.windowId}`)); | |
} |
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.
Oh yeah the getDelayedChannel
will probably be a fix for my problem
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.
Ah yes!
Fixes #175335