Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove PPromise usage from NSFW IPC #53556

Merged
merged 4 commits into from
Jul 5, 2018
Merged

Remove PPromise usage from NSFW IPC #53556

merged 4 commits into from
Jul 5, 2018

Conversation

joaomoreno
Copy link
Member

@joaomoreno joaomoreno commented Jul 4, 2018 •
edited

Related to #53487

@bpasero This removes PPromise usage from the NSFWWatcherService. Give it a good test drive, especially wrt the process crashing and restarting everything again. Everything should work as before.

If good, I'll make exactly the same adoption for Chokidar, since both worlds seem identical.

@joaomoreno joaomoreno added debt Code quality issues engineering VS Code - Build / issue tracking / etc. labels Jul 4, 2018
@joaomoreno joaomoreno added this to the July 2018 milestone Jul 4, 2018
@joaomoreno joaomoreno self-assigned this Jul 4, 2018
@joaomoreno joaomoreno requested a review from bpasero July 4, 2018 20:38
@joaomoreno joaomoreno mentioned this pull request Jul 4, 2018
Merged
11 tasks
@bpasero bpasero self-assigned this Jul 5, 2018
bpasero
bpasero requested changes Jul 5, 2018
View reviewed changes
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joaomoreno left some comments but could not test after all due to this error:

  ERR event is not a function: TypeError: event is not a function
    at file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/event.js:441:20
    at file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/event.js:441:20
    at FileWatcher.startWatching (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/files/node/watcher/nsfw/watcherService.js:51:13)
    at RemoteFileService.FileService.setupFileWatching (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/files/electron-browser/fileService.js:149:99)
    at file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/files/electron-browser/fileService.js:90:23
    at Object.notifySuccess [as _notify] (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/winjs.base.js:1191:59)
    at Object.enter (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/winjs.base.js:867:30)
    at Promise_ctor._run (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/winjs.base.js:1089:29)
    at Promise_ctor._completed (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/winjs.base.js:1057:18)
    at Barrier.open (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/async.js:339:18)
    at LifecycleService.set [as phase] (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/platform/lifecycle/electron-browser/lifecycleService.js:95:54)
    at file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/electron-browser/shell.js:70:50
    at _Base.Class.derive._creator.CompletePromise_done [as done] (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/winjs.base.js:1545:34)
    at Object.notifySuccess [as _notify] (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/winjs.base.js:1201:48)
    at Object.enter (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/winjs.base.js:867:30)
    at Promise_ctor._run (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/winjs.base.js:1089:29)
    at Promise_ctor._completed (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/winjs.base.js:1057:18)
    at argDone (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/winjs.base.js:1834:37)
    at Promise.then.errors.(anonymous function) (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/winjs.base.js:1859:78)
    at Object.notifySuccess [as _notify] (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/winjs.base.js:1191:59)
    at Object.enter (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/winjs.base.js:867:30)
    at _Base.Class.derive._creator._run (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/winjs.base.js:1089:29)
    at _Base.Class.derive._creator._completed (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/winjs.base.js:1057:18)
    at _Base.Class.derive._creator.CompletePromise_then [as then] (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/winjs.base.js:1587:49)

src/vs/base/common/errors.ts Outdated
@@ -242,6 +242,10 @@ export interface IErrorWithActions {
actions?: IAction[];
}

export function isError(obj: any): obj is Error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joaomoreno this seems unrelated? and should also not really be needed with TypeScript

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was referencing the wrong one...

src/vs/base/parts/ipc/node/ipc.cp.ts
@@ -79,6 +79,9 @@ export class Client implements IChannelClient, IDisposable {
private _client: IPCClient;
private channels: { [name: string]: IChannel };

private _onDidProcessExit = new Emitter<{ code: number, signal: string }>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joaomoreno _onDidProcessExit is never getting disposed

src/vs/workbench/services/files/node/watcher/nsfw/nsfwWatcherService.ts Outdated
});
return this._watcherPromise;
watch(options: IWatcherOptions): Event<watcher.IRawFileChange[] | Error> {
this._verboseLogging = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joaomoreno looks like this should be this._verboseLogging = options.verboseLogging (I broke this a long time ago via 38220e3)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, thought you did that on purpose.

src/vs/workbench/services/files/node/watcher/nsfw/watcherService.ts Outdated
@@ -16,15 +15,16 @@ import { IWorkspaceContextService } from 'vs/platform/workspace/common/workspace
import { IConfigurationService } from 'vs/platform/configuration/common/configuration';
import { IDisposable, dispose } from 'vs/base/common/lifecycle';
import { Schemas } from 'vs/base/common/network';
import { isPromiseCanceledError } from 'vs/base/common/errors';
import { filterEvent } from 'vs/base/common/event';
import { isError } from 'util';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joaomoreno did you mean to use errors.isError here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YEAH!

@joaomoreno
Copy link
Member Author

Pushed review comment changes.

bpasero
bpasero requested changes Jul 5, 2018
View reviewed changes
Copy link
Member

@bpasero bpasero left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joaomoreno I tested basic things like file changes and simulating a crash of the watcher and it works fine. however this does not work: if I manually trigger the ENOSPC error after a short timeout from here I do not get a notification. Compared to stable:

image

@joaomoreno
Copy link
Member Author

Great catch. Pushed fix!

@bpasero bpasero removed their assignment Jul 5, 2018
bpasero
bpasero approved these changes Jul 5, 2018
View reviewed changes
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@joaomoreno joaomoreno merged commit 4f833b4 into master Jul 5, 2018
@joaomoreno joaomoreno deleted the joao/remove-ppromise-nsfw branch July 5, 2018 10:06
@joaomoreno joaomoreno mentioned this pull request Jul 5, 2018
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bpasero bpasero bpasero approved these changes

@nzec nzec nzec approved these changes

Assignees

@joaomoreno joaomoreno

Labels
debt Code quality issues engineering VS Code - Build / issue tracking / etc.
Projects
None yet
Milestone
July 2018
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@joaomoreno @bpasero @nzec