Skip to content

Commit 203251d

Browse files
nytaminnytamin
committed
fix: revert use of delayRemove-json-writer for HTTP-Proxy
Also add more debug logging to json-writer
1 parent 1dac69d commit 203251d

3 files changed

Lines changed: 71 additions & 31 deletions

File tree

‎shared/packages/worker/src/worker/accessorHandlers/httpProxy.ts‎

Lines changed: 43 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,11 @@ import { defaultCheckHandleRead, defaultCheckHandleWrite, defaultDoYouSupportAcc
3333
import { GenericFileOperationsHandler } from './lib/GenericFileOperations'
3434
import { JSONWriteFilesBestEffortHandler } from './lib/json-write-file'
3535
import { GenericFileHandler } from './lib/GenericFileHandler'
36+
import { PassThrough } from 'stream'
37+
38+
// Feature flag to disable delayed removal for HTTP-Proxy accessor
39+
// Due to issues with acquiring file lock for the json file...
40+
const ENABLE_HTTP_PROXY_DELAY_REMOVAL = false
3641

3742
/** Accessor handle for accessing files in HTTP- */
3843
export class HTTPProxyAccessorHandle<Metadata> extends GenericAccessorHandle<Metadata> {
@@ -45,7 +50,7 @@ export class HTTPProxyAccessorHandle<Metadata> extends GenericAccessorHandle<Met
4550
private workOptions: Expectation.WorkOptions.RemoveDelay
4651
private accessor: AccessorOnPackage.HTTPProxy
4752

48-
public fileHandler: GenericFileOperationsHandler
53+
public fileHandler: GenericFileOperationsHandler | undefined = undefined
4954

5055
constructor(arg: AccessorConstructorProps<AccessorOnPackage.HTTPProxy>) {
5156
super({
@@ -65,28 +70,30 @@ export class HTTPProxyAccessorHandle<Metadata> extends GenericAccessorHandle<Met
6570
if (this.workOptions.removeDelay && typeof this.workOptions.removeDelay !== 'number')
6671
throw new Error('Bad input data: workOptions.removeDelay is not a number!')
6772

68-
const fileHandler: GenericFileHandler = {
69-
logOperation: this.logOperation.bind(this),
70-
unlinkIfExists: this.deletePackageIfExists.bind(this),
71-
getFullPath: this.getFullPath.bind(this),
72-
getMetadataPath: this.getMetadataPath.bind(this),
73-
fileExists: this.fileExists.bind(this),
74-
readFile: this.readFile.bind(this),
75-
readFileIfExists: this.readFileIfExists.bind(this),
76-
writeFile: this.writeFile.bind(this),
77-
listFilesInDir: this.listFilesInDir.bind(this),
78-
removeDirIfExists: this.removeDirIfExists.bind(this),
79-
rename: this.rename.bind(this),
73+
if (ENABLE_HTTP_PROXY_DELAY_REMOVAL) {
74+
const fileHandler: GenericFileHandler = {
75+
logOperation: this.logOperation.bind(this),
76+
unlinkIfExists: this.deletePackageIfExists.bind(this),
77+
getFullPath: this.getFullPath.bind(this),
78+
getMetadataPath: this.getMetadataPath.bind(this),
79+
fileExists: this.fileExists.bind(this),
80+
readFile: this.readFile.bind(this),
81+
readFileIfExists: this.readFileIfExists.bind(this),
82+
writeFile: this.writeFile.bind(this),
83+
listFilesInDir: this.listFilesInDir.bind(this),
84+
removeDirIfExists: this.removeDirIfExists.bind(this),
85+
rename: this.rename.bind(this),
86+
}
87+
const jsonWriter = this.worker.cacheData(this.type, 'jsonWriter', () => {
88+
return new JSONWriteFilesBestEffortHandler(fileHandler, this.worker.logger)
89+
})
90+
this.fileHandler = new GenericFileOperationsHandler(
91+
fileHandler,
92+
jsonWriter,
93+
this.workOptions,
94+
this.worker.logger
95+
)
8096
}
81-
const jsonWriter = this.worker.cacheData(this.type, 'jsonWriter', () => {
82-
return new JSONWriteFilesBestEffortHandler(fileHandler, this.worker.logger)
83-
})
84-
this.fileHandler = new GenericFileOperationsHandler(
85-
fileHandler,
86-
jsonWriter,
87-
this.workOptions,
88-
this.worker.logger
89-
)
9097
}
9198
static doYouSupportAccess(worker: BaseWorker, accessor: AccessorOnPackage.Any): boolean {
9299
return defaultDoYouSupportAccess(worker, accessor)
@@ -169,10 +176,15 @@ export class HTTPProxyAccessorHandle<Metadata> extends GenericAccessorHandle<Met
169176
return this.convertHeadersToVersion(header.headers)
170177
}
171178
async ensurePackageFulfilled(): Promise<void> {
172-
await this.fileHandler.clearPackageRemoval(this.filePath)
179+
// N/A
180+
// await this.fileHandler.clearPackageRemoval(this.filePath)
173181
}
174182
async removePackage(reason: string): Promise<void> {
175-
await this.fileHandler.handleRemovePackage(this.filePath, this.packageName, reason)
183+
if (ENABLE_HTTP_PROXY_DELAY_REMOVAL && this.fileHandler) {
184+
await this.fileHandler.handleRemovePackage(this.filePath, this.packageName, reason)
185+
} else {
186+
await this.deletePackageIfExists(this.fullUrl)
187+
}
176188
}
177189
async getPackageReadStream(): Promise<PackageReadStream> {
178190
const fetch = fetchWithController(this.fullUrl)
@@ -239,7 +251,9 @@ export class HTTPProxyAccessorHandle<Metadata> extends GenericAccessorHandle<Met
239251
operationName: string,
240252
source: string | GenericAccessorHandle<any>
241253
): Promise<PackageOperation> {
242-
await this.fileHandler.clearPackageRemoval(this.filePath)
254+
if (ENABLE_HTTP_PROXY_DELAY_REMOVAL && this.fileHandler) {
255+
await this.fileHandler.clearPackageRemoval(this.filePath)
256+
}
243257
return this.logWorkOperation(operationName, source, this.packageName)
244258
}
245259
async finalizePackage(operation: PackageOperation): Promise<void> {
@@ -434,7 +448,7 @@ export class HTTPProxyAccessorHandle<Metadata> extends GenericAccessorHandle<Met
434448
if (this.isBadHTTPResponseCode(result.status)) {
435449
const text = await result.text()
436450
throw new Error(
437-
`storeJSON: Bad response: [${result.status}]: ${result.statusText}, POST ${fullPath}, ${text}`
451+
`writeFile: Bad response: [${result.status}]: ${result.statusText}, POST ${fullPath}, ${text}`
438452
)
439453
}
440454
}
@@ -452,7 +466,9 @@ export class HTTPProxyAccessorHandle<Metadata> extends GenericAccessorHandle<Met
452466
})
453467
if (this.isBadHTTPResponseCode(result.status)) {
454468
const text = await result.text()
455-
throw new Error(`storeJSON: Bad response: [${result.status}]: ${result.statusText}, GET /packages ${text}`)
469+
throw new Error(
470+
`listFilesInDir: Bad response: [${result.status}]: ${result.statusText}, GET /packages ${text}`
471+
)
456472
}
457473

458474
// from http-server:

‎shared/packages/worker/src/worker/accessorHandlers/lib/__tests__/json-write-file.spec.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,13 @@ import * as path from 'path'
66
const logger = {
77
error: jest.fn((message: string) => console.log('ERROR', message)),
88
warn: jest.fn((message: string) => console.log('WARNING', message)),
9-
debug: jest.fn((message: string) => console.log('DEBUG', message)),
9+
debug: jest.fn((message: string) => {
10+
// suppress noisy log
11+
if (message.includes('File is already locked')) return
12+
if (message.includes('File was locked by someone else')) return
13+
if (message.includes('Unable to parse Lock file content')) return
14+
console.log('DEBUG', message)
15+
}),
1016
silly: jest.fn((message: string) => console.log('SILLY', message)),
1117
category: () => logger,
1218
}

‎shared/packages/worker/src/worker/accessorHandlers/lib/json-write-file.ts‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,8 @@ export class JSONWriteFilesLockHandler extends JSONWriteHandler {
180180
continue
181181
} else if ((e as any).code === 'ELOCKED') {
182182
// Already locked, try again later:
183+
this.logger.debug(`File is already locked, trying again later...`)
184+
183185
await this.sleep(this.RETRY_TIMEOUT)
184186
continue
185187
} else {
@@ -201,6 +203,7 @@ export class JSONWriteFilesLockHandler extends JSONWriteHandler {
201203
} else {
202204
if (lockCompromisedError) {
203205
// The lock was compromised. Try again:
206+
this.logger.warn(`File lock was compromised, trying again later... (${lockCompromisedError})`)
204207
continue
205208
}
206209

@@ -302,15 +305,23 @@ export class JSONWriteFilesBestEffortHandler extends JSONWriteHandler {
302305
if (lockFileContentStr) {
303306
try {
304307
lockFileContent = JSON.parse(lockFileContentStr)
305-
} catch {
308+
} catch (e) {
306309
// ignore
310+
this.logger.debug(`Unable to parse Lock file content: (${e}, ${lockFileContentStr})`)
307311
}
308312
}
309313
if (lockFileContent) {
310314
const lockedDate = new Date(lockFileContent.date)
315+
const lockAge = Date.now() - lockedDate.getTime()
316+
317+
if (Math.abs(lockAge) < 10 * 1000) {
318+
// The lock file is in place and rather new,
319+
// wait and try again later:
320+
321+
this.logger.debug(
322+
`File is already locked, trying again later... (date: ${lockFileContent.date}, age: ${lockAge} ms)`
323+
)
311324

312-
if (Math.abs(Date.now() - lockedDate.getTime()) < 10 * 1000) {
313-
// The lock file is not too old, so we can use it.
314325
// Try again later:
315326
await this.sleep(this.RETRY_TIMEOUT)
316327
continue
@@ -335,6 +346,13 @@ export class JSONWriteFilesBestEffortHandler extends JSONWriteHandler {
335346
const checkLockFileContent = await this.readIfExists(fileLockPath)
336347
if (checkLockFileContent !== myLockFileContents) {
337348
// Dang it, someone else got the lock before us.
349+
350+
this.logger.debug(
351+
`File was locked by someone else, trying again later... (${JSON.stringify(
352+
checkLockFileContent
353+
)} vs ${JSON.stringify(myLockFileContents)})`
354+
)
355+
338356
// Try again later:
339357
await this.sleep(this.RETRY_TIMEOUT)
340358
continue

0 commit comments

Comments
 (0)