Skip to content

Commit

Permalink
fix #108168
Browse files Browse the repository at this point in the history
  • Loading branch information
@sandy081
sandy081 committed Oct 8, 2020
1 parent b90805e commit 6bebcbb
Show file tree
Hide file tree
Showing 6 changed files with 25 additions and 10 deletions.
4 changes: 3 additions & 1 deletion src/vs/platform/log/common/fileLogService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,9 @@ export class FileLogService extends AbstractLogService implements ILogService {
}

private async initialize(): Promise<void> {
await this.fileService.createFile(this.resource);
if (!await this.fileService.exists(this.resource)) {

This comment has been minimized.

Sign in to view

Copy link
@bpasero

bpasero Oct 10, 2020

Member

@sandy081 if this is code that may block startup, a faster variant is to put the createFile around a try-catch block. Otherwise you pay an extra file access.

This comment has been minimized.

Sign in to view

Copy link
@sandy081

sandy081 Oct 19, 2020

Author Member

Is there an error code to know if the error is about file already exists?

This comment has been minimized.

Sign in to view

Copy link
@bpasero

bpasero Oct 19, 2020

Member

@sandy081

vscode/src/vs/workbench/contrib/files/browser/editors/textFileEditor.ts

Line 163 in 84afb9f

if ((<FileOperationError>error).fileOperationResult === FileOperationResult.FILE_NOT_FOUND && isValidBasename(basename(input.preferredResource))) {

This comment has been minimized.

Sign in to view

Copy link
@sandy081

sandy081 Oct 19, 2020

Author Member

Sorry. I am not sure if error code FILE_NOT_FOUND works for me. I am trying to create a file and an error is thrown if file exists already. From debug, I am getting FILE_MODIFIED_SINCE error from the service. Is it the right error code?

This comment has been minimized.

Sign in to view

Copy link
@bpasero

bpasero Oct 19, 2020

Member

@sandy081 yes that should work, it is thrown here:

vscode/src/vs/platform/files/common/fileService.ts

Line 309 in bb3aaa7

throw new FileOperationError(localize('fileExists', "Unable to create file '{0}' that already exists when overwrite flag is not set", this.resourceForError(resource)), FileOperationResult.FILE_MODIFIED_SINCE, options);

This comment has been minimized.

Sign in to view

Copy link
@sandy081

sandy081 Oct 23, 2020

Author Member

@DBJDBJ FYI

This comment has been minimized.

Sign in to view

Copy link
@DBJDBJ

DBJDBJ Oct 24, 2020 •
edited
Loading

Basically, the theory goes:

"exceptions are just and only for exceptional situations", while returns are to be descriptive enough to be able to code a bit more involved return handling logic.

One does always finish with a "special structure" returned that will allow for "returns handling" which is not "error handling". Especially useful in situations like this where "the thing returned is not an error".

IO situations are a good example. Thus I might suggest returning a structure, something like { return_value, status_code } ... combination of the two allows for nontrivial passing out of the function and processing on the caller side. I might prefer this to exceptions:

return FileOperationOutcome (
// you do not want to spend CPU cycles on localized message making
// and returning, just pass the "message code" that caller might or might not use
// not all situations require human-readable messages 
message_code( resource ),  
// this is good
// this is where you pass the status to the caller
FileOperationResult.FILE_MODIFIED_SINCE, 
// this looks like it might be better as an
// property of the file operations result
// unless it has a completely decoupled logic
options
);

Technically exception throwing catching mechanisms do lead to bigger and slower executables. But in the world of Electron users, that is hardly relevant.

Caveat Emptor: I am C/C++ head, although with a rich javascript lineage :) I am not helping. Changing a fundamental API right now is probably not an option.
await this.fileService.createFile(this.resource);
}
}

private _log(level: LogLevel, message: string): void {
Expand Down
3 changes: 3 additions & 0 deletions src/vs/workbench/services/environment/browser/environmentService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,9 @@ export class BrowserWorkbenchEnvironmentService implements IWorkbenchEnvironment
@memoize
get serviceMachineIdResource(): URI { return joinPath(this.userRoamingDataHome, 'machineid'); }

@memoize
get extHostLogsPath(): URI { return joinPath(this.options.logsPath, 'exthost'); }

private _extensionHostDebugEnvironment: IExtensionHostDebugEnvironment | undefined = undefined;
get debugExtensionHost(): IExtensionHostDebugParams {
if (!this._extensionHostDebugEnvironment) {
Expand Down
1 change: 1 addition & 0 deletions src/vs/workbench/services/environment/common/environmentService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export interface IWorkbenchEnvironmentService extends IEnvironmentService {
readonly logFile: URI;
readonly backupWorkspaceHome?: URI;

readonly extHostLogsPath: URI;
readonly logExtensionHostCommunication?: boolean;
readonly extensionEnabledProposedApi?: string[];

Expand Down
2 changes: 0 additions & 2 deletions src/vs/workbench/services/environment/electron-sandbox/environmentService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import { IWorkbenchConfiguration, IWorkbenchEnvironmentService } from 'vs/workbench/services/environment/common/environmentService';
import { INativeWindowConfiguration } from 'vs/platform/windows/common/windows';
import { INativeEnvironmentService } from 'vs/platform/environment/common/environment';
import { URI } from 'vs/base/common/uri';
import { createDecorator } from 'vs/platform/instantiation/common/instantiation';

export const INativeWorkbenchEnvironmentService = createDecorator<INativeWorkbenchEnvironmentService>('nativeEnvironmentService');
Expand All @@ -27,7 +26,6 @@ export interface INativeWorkbenchEnvironmentService extends IWorkbenchEnvironmen
readonly execPath: string;

readonly log?: string;
readonly extHostLogsPath: URI;

// TODO@ben this is a bit ugly
updateBackupPath(newPath: string | undefined): void;
Expand Down
2 changes: 1 addition & 1 deletion src/vs/workbench/services/extensions/browser/webWorkerExtensionHost.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ export class WebWorkerExtensionHost extends Disposable implements IExtensionHost
this._isTerminating = false;
this._protocolPromise = null;
this._protocol = null;
this._extensionHostLogsLocation = URI.file(this._environmentService.logsPath).with({ scheme: this._environmentService.logFile.scheme });
this._extensionHostLogsLocation = joinPath(this._environmentService.extHostLogsPath, 'webWorker');
this._extensionHostLogFile = joinPath(this._extensionHostLogsLocation, `${ExtensionHostLogFileName}.log`);
}

Expand Down
23 changes: 17 additions & 6 deletions src/vs/workbench/services/output/common/outputChannelModel.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,12 +209,23 @@ class FileOutputChannelModel extends AbstractFileOutputChannelModel implements I
}

loadModel(): Promise<ITextModel> {
this.loadModelPromise = this.fileService.readFile(this.file, { position: this.startOffset })
.then(content => {
this.endOffset = this.startOffset + content.value.byteLength;
this.etag = content.etag;
return this.createModel(content.value.toString());
});
this.loadModelPromise = new Promise<ITextModel>(async (c, e) => {
try {
let content = '';
if (await this.fileService.exists(this.file)) {
const fileContent = await this.fileService.readFile(this.file, { position: this.startOffset });
this.endOffset = this.startOffset + fileContent.value.byteLength;
this.etag = fileContent.etag;
content = fileContent.value.toString();
} else {
this.startOffset = 0;
this.endOffset = 0;
}
c(this.createModel(content));
} catch (error) {
e(error);
}
});
return this.loadModelPromise;
}

Expand Down

0 comments on commit 6bebcbb

Please sign in to comment.