files - avoid read/write with pos and prefer null (fix #73884)

src/vs/platform/files/node/diskFileSystemProvider.ts

@@ -148,6 +148,8 @@ export class DiskFileSystemProvider extends Disposable implements IFileSystemPro
 		}
 	}
 
+	private mapHandleToPos: Map<number, number> = new Map();
+
 	private writeHandles: Set<number> = new Set();
 	private canFlush: boolean = true;
 
@@ -187,6 +189,13 @@ export class DiskFileSystemProvider extends Disposable implements IFileSystemPro
 
 			const handle = await promisify(open)(filePath, flags);
 
+			// remember this handle to track file position of the handle
+			// we init the position to 0 since the file descriptor was
+			// just created and the position was not moved so far (see
+			// also http://man7.org/linux/man-pages/man2/open.2.html -
+			// "The file offset is set to the beginning of the file.")
+			this.mapHandleToPos.set(handle, 0);
+
 			// remember that this handle was used for writing
 			if (opts.create) {
 				this.writeHandles.add(handle);
@@ -200,6 +209,10 @@ export class DiskFileSystemProvider extends Disposable implements IFileSystemPro
 
 	async close(fd: number): Promise<void> {
 		try {
+
+			// remove this handle from map of positions
+			this.mapHandleToPos.delete(fd);
+
 			// if a handle is closed that was used for writing, ensure
 			// to flush the contents to disk if possible.
 			if (this.writeHandles.delete(fd) && this.canFlush) {
@@ -220,15 +233,81 @@ export class DiskFileSystemProvider extends Disposable implements IFileSystemPro
 	}
 
 	async read(fd: number, pos: number, data: Uint8Array, offset: number, length: number): Promise<number> {
+		const normalizedPos = this.normalizePos(fd, pos);
+
+		let bytesRead: number | null = null;
 		try {
-			const result = await promisify(read)(fd, data, offset, length, pos);
+			const result = await promisify(read)(fd, data, offset, length, normalizedPos);
+
 			if (typeof result === 'number') {
-				return result; // node.d.ts fail
+				bytesRead = result; // node.d.ts fail
+			} else {
+				bytesRead = result.bytesRead;
 			}
 
-			return result.bytesRead;
+			return bytesRead;
 		} catch (error) {
 			throw this.toFileSystemProviderError(error);
+		} finally {
+			this.updatePos(fd, normalizedPos, bytesRead);
+		}
+	}
+
+	private normalizePos(fd: number, pos: number): number | null {
+
+		// when calling fs.read/write we try to avoid passing in the "pos" argument and
+		// rather prefer to pass in "null" because this avoids an extra seek(pos)
+		// call that in some cases can even fail (e.g. when opening a file over FTP -
+		// see https://github.com/microsoft/vscode/issues/73884).
+		//
+		// as such, we compare the passed in position argument with our last known
+		// position for the file descriptor and use "null" if they match.
+		if (pos === this.mapHandleToPos.get(fd)) {
+			return null;
+		}
+
+		return pos;
+	}
+
+	private updatePos(fd: number, pos: number | null, bytesLength: number | null): void {
+		const lastKnownPos = this.mapHandleToPos.get(fd);
+		if (typeof lastKnownPos === 'number') {
+
+			// pos !== null signals that previously a position was used that is
+			// not null. node.js documentation explains, that in this case
+			// the internal file pointer is not moving and as such we do not move
+			// our position pointer.
+			//
+			// Docs: "If position is null, data will be read from the current file position,
+			// and the file position will be updated. If position is an integer, the file position
+			// will remain unchanged."
+			if (typeof pos === 'number') {
+				// do not modify the position
+			}
+
+			// bytesLength = number is a signal that the read/write operation was
+			// successful and as such we need to advance the position in the Map
+			//
+			// Docs (http://man7.org/linux/man-pages/man2/read.2.html):
+			// "On files that support seeking, the read operation commences at the
+			// file offset, and the file offset is incremented by the number of
+			// bytes read."
+			//
+			// Docs (http://man7.org/linux/man-pages/man2/write.2.html):
+			// "For a seekable file (i.e., one to which lseek(2) may be applied, for
+			// example, a regular file) writing takes place at the file offset, and
+			// the file offset is incremented by the number of bytes actually
+			// written."
+			else if (typeof bytesLength === 'number') {
+				this.mapHandleToPos.set(fd, lastKnownPos + bytesLength);
+			}
+
+			// bytesLength = null signals an error in the read/write operation
+			// and as such we drop the handle from the Map because the position
+			// is unspecificed at this point.
+			else {
+				this.mapHandleToPos.delete(fd);
+			}
 		}
 	}
 
@@ -240,15 +319,23 @@ export class DiskFileSystemProvider extends Disposable implements IFileSystemPro
 	}
 
 	private async doWrite(fd: number, pos: number, data: Uint8Array, offset: number, length: number): Promise<number> {
+		const normalizedPos = this.normalizePos(fd, pos);
+
+		let bytesWritten: number | null = null;
 		try {
-			const result = await promisify(write)(fd, data, offset, length, pos);
+			const result = await promisify(write)(fd, data, offset, length, normalizedPos);
+
 			if (typeof result === 'number') {
-				return result; // node.d.ts fail
+				bytesWritten = result; // node.d.ts fail
+			} else {
+				bytesWritten = result.bytesWritten;
 			}
 
-			return result.bytesWritten;
+			return bytesWritten;
 		} catch (error) {
 			throw this.toFileSystemProviderError(error);
+		} finally {
+			this.updatePos(fd, normalizedPos, bytesWritten);
 		}
 	}
 
@@ -441,14 +528,17 @@ export class DiskFileSystemProvider extends Disposable implements IFileSystemPro
 						watcherOptions?: IWatcherOptions
 					): WindowsWatcherService | UnixWatcherService | NsfwWatcherService
 				};
-				let watcherOptions = undefined;
 
+				let watcherOptions: IWatcherOptions | undefined = undefined;
+
+				// requires a polling watcher
 				if (this.watcherOptions && this.watcherOptions.usePolling) {
-					// requires a polling watcher
 					watcherImpl = UnixWatcherService;
 					watcherOptions = this.watcherOptions;
-				} else {
-					// Single Folder Watcher
+				}
+
+				// Single Folder Watcher
+				else {
 					if (this.recursiveFoldersToWatch.length === 1) {
 						if (isWindows) {
 							watcherImpl = WindowsWatcherService;
@@ -471,6 +561,7 @@ export class DiskFileSystemProvider extends Disposable implements IFileSystemPro
 						if (msg.type === 'error') {
 							this._onDidWatchErrorOccur.fire(msg.message);
 						}
+
 						this.logService[msg.type](msg.message);
 					},
 					this.logService.getLevel() === LogLevel.Trace,
@@ -478,7 +569,7 @@ export class DiskFileSystemProvider extends Disposable implements IFileSystemPro
 				);
 
 				if (!this.recursiveWatcherLogLevelListener) {
-					this.recursiveWatcherLogLevelListener = this.logService.onDidChangeLogLevel(_ => {
+					this.recursiveWatcherLogLevelListener = this.logService.onDidChangeLogLevel(() => {
 						if (this.recursiveWatcher) {
 							this.recursiveWatcher.setVerboseLogging(this.logService.getLevel() === LogLevel.Trace);
 						}
@@ -496,11 +587,13 @@ export class DiskFileSystemProvider extends Disposable implements IFileSystemPro
 				if (msg.type === 'error') {
 					this._onDidWatchErrorOccur.fire(msg.message);
 				}
+
 				this.logService[msg.type](msg.message);
 			},
 			this.logService.getLevel() === LogLevel.Trace
 		);
-		const logLevelListener = this.logService.onDidChangeLogLevel(_ => {
+
+		const logLevelListener = this.logService.onDidChangeLogLevel(() => {
 			watcherService.setVerboseLogging(this.logService.getLevel() === LogLevel.Trace);
 		});
 
@@ -553,4 +646,4 @@ export class DiskFileSystemProvider extends Disposable implements IFileSystemPro
 		dispose(this.recursiveWatcherLogLevelListener);
 		this.recursiveWatcherLogLevelListener = undefined;
 	}
-}
+}