Fix or workaround logical and cosmetic bugs with S3 impl (#1134)

- Add locking for (initial call to) GetChildren and (all calls to) LoadMoreChildren
Both of these append to the internal node cache
Any combination of these calls can possibly be in progress concurrently
The easiest (and probably most correct) behavior is to queue up calls behind a single write lock

- Ignore attempts to load more children when one is already in progress for the node
This can happen if the user double clicks the load more node
The intent was most likely to load a single page
Rather than queueing up 2 pages, it's better to drop the subsequent load command invocations

- Remove leading period from file extension filter
Per electron doc, the extension filter should not contain the leading period
Without this, Windows was shown to duplicate the file extension

- Tweak order of file extension filters
Was shown on mac that the first item will be the one selected by default

- Use hardcoded SVGs when the user has Seti file icon theme enabled
Indentation of child nodes gets messed up when the parent node doesn't have an icon
microsoft/vscode#85654

This is the case for the very common Seti file icon theme
Switching to SVG works around the issue for Seti

Other themes may also not have folder icons however there is no way
to detect this currently and the thought is that this is quite uncommon

The pros of using the native icon theme outweigh the cons here

- Clear the AWS SDK S3 bucket region cache before invocations
S3 maintains an internal cache of region to bucket name as an optimization
however this cache does not contain the partition

Bucket names are not unique across partitions

This can cause the wrong region to be selected when the
user switches between partitions that contain buckets with the same name

- Workaround logging errors when Error.name is a number
e.g. AWS SDK for JS can sometimes set the error name to the error code number (like 404)

Node.inspect (used by the logger) has a bug where it expects the name to be a string
and will actually throw an Error otherwise

VSCode's ES5 type definitions and lodash also make this expectation

The spec seems to make no mention of the string requirement

Additionally, Node fixed the Error in v12.14.1
nodejs/node#30572

However, VSCode uses an older version of Node without the fix

src/awsexplorer/childNodeLoader.ts

@@ -7,6 +7,9 @@ import { AWSTreeNodeBase } from '../shared/treeview/nodes/awsTreeNodeBase'
 import { LoadMoreNode } from '../shared/treeview/nodes/loadMoreNode'
 import { MoreResultsNode } from './moreResultsNode'
 import { ChildNodeCache } from './childNodeCache'
+import * as AsyncLock from 'async-lock'
+
+const LOCK_KEY = 'ChildNodeLoader'
 
 export interface ChildNodePage {
     newChildren: AWSTreeNodeBase[]
@@ -19,6 +22,7 @@ export interface ChildNodePage {
 export class ChildNodeLoader {
     private readonly loadPage: (continuationToken: string | undefined) => Promise<ChildNodePage>
     private readonly moreResults: MoreResultsNode
+    private readonly loadChildrenLock: AsyncLock
     private cache: ChildNodeCache
 
     public constructor(
@@ -27,28 +31,32 @@ export class ChildNodeLoader {
     ) {
         this.loadPage = loadPage
         this.moreResults = new MoreResultsNode(parent)
+        this.loadChildrenLock = new AsyncLock()
         this.cache = new ChildNodeCache()
     }
 
     /**
      * Gets the initial or previously-loaded children.
      */
     public async getChildren(): Promise<AWSTreeNodeBase[]> {
-        if (!this.initialChildrenLoaded()) {
-            return this.loadInitialChildren()
-        }
-
+        await this.loadMoreChildrenIf(() => !this.initialChildrenLoaded())
         return this.getExistingChildren()
     }
 
     /**
-     * Loads and returns more children.
+     * Loads and appends a new page of children.
+     *
+     * If there is no new page of children, has no effect.
      */
-    public async loadMoreChildren(): Promise<AWSTreeNodeBase[]> {
-        const newPage = await this.loadPage(this.cache.continuationToken)
+    public async loadMoreChildren(): Promise<void> {
+        return this.loadMoreChildrenIf(() => !this.allChildrenLoaded())
+    }
 
-        this.cache.appendPage(newPage)
-        return this.getExistingChildren()
+    /**
+     * Returns true if a {@link loadMoreChildren} call is in progress.
+     */
+    public isLoadingMoreChildren(): boolean {
+        return this.loadChildrenLock.isBusy(LOCK_KEY)
     }
 
     /**
@@ -62,15 +70,34 @@ export class ChildNodeLoader {
         return !this.cache.isPristine
     }
 
-    private async loadInitialChildren(): Promise<AWSTreeNodeBase[]> {
-        return this.loadMoreChildren()
+    private allChildrenLoaded(): boolean {
+        return this.initialChildrenLoaded() && this.cache.continuationToken === undefined
     }
 
-    private async getExistingChildren(): Promise<AWSTreeNodeBase[]> {
+    private getExistingChildren(): AWSTreeNodeBase[] {
         if (this.cache.continuationToken !== undefined) {
             return [...this.cache.children, this.moreResults]
         }
 
         return this.cache.children
     }
+
+    /**
+     * Prevents multiple concurrent attempts to load next page.
+     *
+     * This can happen if the user double clicks a node that executes a command before the node is hidden.
+     * In this case, the attempts are queued up.
+     *
+     * @param condition a double checked condition that must evaluate to true for the page load to take place.
+     */
+    private async loadMoreChildrenIf(condition: () => boolean): Promise<void> {
+        if (condition()) {
+            return this.loadChildrenLock.acquire(LOCK_KEY, async () => {
+                if (condition()) {
+                    const newPage = await this.loadPage(this.cache.continuationToken)
+                    this.cache.appendPage(newPage)
+                }
+            })
+        }
+    }
 }

src/awsexplorer/commands/loadMoreChildren.ts

@@ -19,8 +19,15 @@ export async function loadMoreChildrenCommand(
     awsExplorer: AwsExplorer,
     window = Window.vscode()
 ): Promise<void> {
+    getLogger().debug('LoadMoreChildren called for %O', node)
+
+    // This can happen if the user double clicks a node that executes this command before the node is hidden
+    if (node.isLoadingMoreChildren()) {
+        getLogger().debug('LoadMoreChildren already in progress. Ignoring.')
+        return
+    }
+
     try {
-        getLogger().debug('LoadMoreChildren called for %O', node)
         await node.loadMoreChildren()
     } catch (e) {
         const logsItem = localize('AWS.generic.message.viewLogs', 'View Logs...')

src/extension.ts

@@ -232,6 +232,12 @@ function initializeIconPaths(context: vscode.ExtensionContext) {
     ext.iconPaths.dark.s3 = context.asAbsolutePath('resources/dark/s3/bucket.svg')
     ext.iconPaths.light.s3 = context.asAbsolutePath('resources/light/s3/bucket.svg')
 
+    ext.iconPaths.dark.folder = context.asAbsolutePath('third-party/resources/from-vscode/dark/folder.svg')
+    ext.iconPaths.light.folder = context.asAbsolutePath('third-party/resources/from-vscode/light/folder.svg')
+
+    ext.iconPaths.dark.file = context.asAbsolutePath('third-party/resources/from-vscode/dark/document.svg')
+    ext.iconPaths.light.file = context.asAbsolutePath('third-party/resources/from-vscode/light/document.svg')
+
     ext.iconPaths.dark.schema = context.asAbsolutePath('resources/dark/schema.svg')
     ext.iconPaths.light.schema = context.asAbsolutePath('resources/light/schema.svg')
 

src/s3/commands/downloadFileAs.ts

@@ -48,12 +48,13 @@ export async function downloadFileAsCommand(node: S3FileNode, window = Window.vs
 }
 
 async function promptForSaveLocation(fileName: string, window: Window): Promise<vscode.Uri | undefined> {
-    const filters: vscode.SaveDialogOptions['filters'] = { 'All files': ['*'] }
-
     const extension = path.extname(fileName)
-    if (extension) {
-        filters[`*${extension}`] = [extension]
-    }
+
+    // Insertion order matters, as it determines the ordering in the filters dropdown
+    // First inserted item is at the top (this should be the extension, if present)
+    const filters: vscode.SaveDialogOptions['filters'] = extension
+        ? { [`*${extension}`]: [extension.slice(1)], 'All Files': ['*'] }
+        : { 'All Files': ['*'] }
 
     const downloadPath = path.join(downloadsDir(), fileName)
     return window.showSaveDialog({

src/s3/explorer/s3BucketNode.ts

@@ -58,8 +58,12 @@ export class S3BucketNode extends AWSTreeNodeBase implements AWSResourceNode, Lo
         })
     }
 
-    public async loadMoreChildren(): Promise<AWSTreeNodeBase[]> {
-        return this.childLoader.loadMoreChildren()
+    public async loadMoreChildren(): Promise<void> {
+        await this.childLoader.loadMoreChildren()
+    }
+
+    public isLoadingMoreChildren(): boolean {
+        return this.childLoader.isLoadingMoreChildren()
     }
 
     public clearChildren(): void {

src/s3/explorer/s3FileNode.ts

@@ -7,9 +7,10 @@ import * as moment from 'moment'
 import * as bytes from 'bytes'
 import * as vscode from 'vscode'
 import { Bucket, DownloadFileRequest, File, S3Client } from '../../shared/clients/s3Client'
+import { ext } from '../../shared/extensionGlobals'
 import { AWSResourceNode } from '../../shared/treeview/nodes/awsResourceNode'
 import { AWSTreeNodeBase } from '../../shared/treeview/nodes/awsTreeNodeBase'
-import { localize } from '../../shared/utilities/vsCodeUtils'
+import { isFileIconThemeSeti, localize } from '../../shared/utilities/vsCodeUtils'
 import { inspect } from 'util'
 
 /**
@@ -40,7 +41,7 @@ export class S3FileNode extends AWSTreeNodeBase implements AWSResourceNode {
             )
             this.description = `${readableSize}, ${readableDate}`
         }
-        this.iconPath = vscode.ThemeIcon.File
+        this.iconPath = fileIconPath()
         this.contextValue = 'awsS3FileNode'
     }
 
@@ -71,3 +72,16 @@ export class S3FileNode extends AWSTreeNodeBase implements AWSResourceNode {
 function formatBytes(numBytes: number): string {
     return bytes(numBytes, { unitSeparator: ' ', decimalPlaces: 0 })
 }
+
+function fileIconPath(): vscode.ThemeIcon | { light: vscode.Uri; dark: vscode.Uri } {
+    // Workaround for https://github.com/microsoft/vscode/issues/85654
+    // Once this is resolved, ThemeIcons can be used for seti as well
+    if (isFileIconThemeSeti()) {
+        return {
+            dark: vscode.Uri.file(ext.iconPaths.dark.file),
+            light: vscode.Uri.file(ext.iconPaths.light.file),
+        }
+    } else {
+        return vscode.ThemeIcon.File
+    }
+}

src/s3/explorer/s3FolderNode.ts

@@ -18,13 +18,14 @@ import { ErrorNode } from '../../shared/treeview/nodes/errorNode'
 import { LoadMoreNode } from '../../shared/treeview/nodes/loadMoreNode'
 import { PlaceholderNode } from '../../shared/treeview/nodes/placeholderNode'
 import { makeChildrenNodes } from '../../shared/treeview/treeNodeUtilities'
-import { localize } from '../../shared/utilities/vsCodeUtils'
+import { isFileIconThemeSeti, localize } from '../../shared/utilities/vsCodeUtils'
 import { ChildNodeLoader } from '../../awsexplorer/childNodeLoader'
 import { ChildNodePage } from '../../awsexplorer/childNodeLoader'
 import { S3FileNode } from './s3FileNode'
 import { inspect } from 'util'
 import { Workspace } from '../../shared/vscode/workspace'
 import { getLogger } from '../../shared/logger'
+import { ext } from '../../shared/extensionGlobals'
 
 /**
  * Represents a folder in an S3 bucket that may contain subfolders and/or objects.
@@ -40,7 +41,7 @@ export class S3FolderNode extends AWSTreeNodeBase implements AWSResourceNode, Lo
     ) {
         super(folder.name, vscode.TreeItemCollapsibleState.Collapsed)
         this.tooltip = folder.path
-        this.iconPath = vscode.ThemeIcon.Folder
+        this.iconPath = folderIconPath()
         this.contextValue = 'awsS3FolderNode'
         this.childLoader = new ChildNodeLoader(this, token => this.loadPage(token))
     }
@@ -55,8 +56,12 @@ export class S3FolderNode extends AWSTreeNodeBase implements AWSResourceNode, Lo
         })
     }
 
-    public async loadMoreChildren(): Promise<AWSTreeNodeBase[]> {
-        return this.childLoader.loadMoreChildren()
+    public async loadMoreChildren(): Promise<void> {
+        await this.childLoader.loadMoreChildren()
+    }
+
+    public isLoadingMoreChildren(): boolean {
+        return this.childLoader.isLoadingMoreChildren()
     }
 
     public clearChildren(): void {
@@ -116,3 +121,16 @@ export class S3FolderNode extends AWSTreeNodeBase implements AWSResourceNode, Lo
         return this.workspace.getConfiguration('aws').get<number>('s3.maxItemsPerPage')
     }
 }
+
+function folderIconPath(): vscode.ThemeIcon | { light: vscode.Uri; dark: vscode.Uri } {
+    // Workaround for https://github.com/microsoft/vscode/issues/85654
+    // Once this is resolved, ThemeIcons can be used for seti as well
+    if (isFileIconThemeSeti()) {
+        return {
+            dark: vscode.Uri.file(ext.iconPaths.dark.folder),
+            light: vscode.Uri.file(ext.iconPaths.light.folder),
+        }
+    } else {
+        return vscode.ThemeIcon.Folder
+    }
+}

src/shared/clients/defaultS3Client.ts

@@ -3,7 +3,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import { S3 } from 'aws-sdk'
+import { S3, AWSError } from 'aws-sdk'
 import * as _ from 'lodash'
 import * as mime from 'mime-types'
 import * as path from 'path'
@@ -263,8 +263,6 @@ export class DefaultS3Client implements S3Client {
      *
      * Note that although there is an S3#GetBucketLocation API,
      * this is the suggested method of obtaining the region.
-     *
-     * @throws Error if there is an error calling S3.
      */
     private async lookupRegion(bucketName: string, s3: S3): Promise<string> {
         getLogger().debug('LookupRegion called for bucketName: %s', bucketName)
@@ -275,8 +273,7 @@ export class DefaultS3Client implements S3Client {
             getLogger().debug('LookupRegion returned region: %s', region)
             return region
         } catch (e) {
-            getLogger().error('Failed to find region for bucket %s: %O', bucketName, e)
-            throw e
+            return (e as AWSError).region
         }
     }
 }
@@ -359,9 +356,20 @@ function buildArn({ partitionId, bucketName, key }: { partitionId: string; bucke
 }
 
 async function createSdkClient(regionCode: string): Promise<S3> {
+    clearInternalBucketCache()
+
     return await ext.sdkClientBuilder.createAndConfigureServiceClient(
         options => new S3(options),
         { computeChecksums: true },
         regionCode
     )
 }
+
+/**
+ * Bucket region is cached across invocations without regard to partition
+ * If partition changes with same bucket name in both partitions, cache is incorrect
+ * @see https://github.com/aws/aws-sdk-js/blob/16a799c0681c01dcafa7b30be5f16894861b3a32/lib/services/s3.js#L919-L924
+ */
+function clearInternalBucketCache(): void {
+    ;(S3.prototype as any).bucketRegionCache = {}
+}

src/shared/extensionGlobals.ts

@@ -51,6 +51,8 @@ export interface IconPaths {
     settings: string
     registry: string
     s3: string
+    folder: string
+    file: string
     schema: string
     cloudWatchLogGroup: string
 }
@@ -63,6 +65,8 @@ function makeIconPathsObject(): IconPaths {
         settings: '',
         registry: '',
         s3: '',
+        folder: '',
+        file: '',
         schema: '',
         statemachine: '',
         cloudWatchLogGroup: '',

src/shared/logger/winstonToolkitLogger.ts

@@ -8,7 +8,7 @@ import * as winston from 'winston'
 import { ConsoleLogTransport } from './consoleLogTransport'
 import { Logger, LogLevel } from './logger'
 import { OutputChannelTransport } from './outputChannelTransport'
-import { isError } from 'lodash'
+import { isError } from 'util'
 
 export class WinstonToolkitLogger implements Logger, vscode.Disposable {
     private readonly logger: winston.Logger
@@ -86,10 +86,27 @@ export class WinstonToolkitLogger implements Logger, vscode.Disposable {
             throw new Error('Cannot write to disposed logger')
         }
 
+        meta.filter(item => isError(item)).forEach(error => coerceNameToString(error))
+
         if (isError(message)) {
+            coerceNameToString(message)
             this.logger.log(level, '%O', message, ...meta)
         } else {
             this.logger.log(level, message, ...meta)
         }
     }
 }
+
+/**
+ * Workaround for logging Errors with a name that's not a string.
+ *
+ * e.g. AWS SDK for JS can sometimes set the error name to the error code number (like 404).
+ *
+ * Fixed in Node v12.14.1, can be removed once VSCode uses this version.
+ * @see https://github.com/nodejs/node/issues/30572
+ */
+function coerceNameToString(error: any): void {
+    if (typeof error.name === 'number') {
+        error.name = String(error.name)
+    }
+}

src/shared/treeview/nodes/loadMoreNode.ts

@@ -3,8 +3,6 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import { AWSTreeNodeBase } from './awsTreeNodeBase'
-
 /**
  * Represents a Node that has the ability to load more results.
  *
@@ -35,12 +33,13 @@ export interface LoadMoreNode {
      *
      * After that, the Node must be refreshed to trigger a call to getChildren() (i.e. "return your loaded children").
      * That will correctly display the (existing and) new results that were appended to the cache.
-     *
-     * Callers must await the Promise before refreshing the node to avoid a race condition.
-     *
-     * @returns the existing and new children of the node.
      */
-    loadMoreChildren(): Promise<AWSTreeNodeBase[]>
+    loadMoreChildren(): Promise<void>
+
+    /**
+     * Returns true if a {@link loadMoreChildren} call is in progress.
+     */
+    isLoadingMoreChildren(): boolean
 
     /**
      * Clears all children from the Node's cache.

src/shared/utilities/vsCodeUtils.ts

@@ -116,3 +116,8 @@ export function getChannelLogger(channel: vscode.OutputChannel): ChannelLogger {
             }),
     })
 }
+
+export function isFileIconThemeSeti(): boolean {
+    const iconTheme = vscode.workspace.getConfiguration('workbench').get('iconTheme')
+    return !iconTheme || iconTheme === 'vs-seti'
+}