From 378fe2de1cff5721f12028d2c3e9b6919286c2a9 Mon Sep 17 00:00:00 2001 From: Yang Yang Date: Fri, 9 Aug 2019 13:25:03 +0800 Subject: [PATCH] [Code] fixed the issue that the repository can not be deleted in some cases. (#42841) * [Code] fixed the issue that the repository can not be deleted in some cases. * [Code] catch the rejected exception while cancelling the job, and added a unit test for CancellationService when the promise of the job get rejected. * [Code] refined tests for CancellationService by deferred completion of the promise. --- .../server/queue/cancellation_service.test.ts | 46 ++++++++++++++++--- .../code/server/queue/cancellation_service.ts | 13 ++++-- .../plugins/code/server/routes/repository.ts | 13 ++++-- 3 files changed, 58 insertions(+), 14 deletions(-) diff --git a/x-pack/legacy/plugins/code/server/queue/cancellation_service.test.ts b/x-pack/legacy/plugins/code/server/queue/cancellation_service.test.ts index c1ec6240fcc16..735a32a05594e 100644 --- a/x-pack/legacy/plugins/code/server/queue/cancellation_service.test.ts +++ b/x-pack/legacy/plugins/code/server/queue/cancellation_service.test.ts @@ -25,12 +25,46 @@ test('Register and cancel cancellation token', async () => { const cancelSpy = sinon.spy(); token.cancel = cancelSpy; - await service.registerCancelableIndexJob( - repoUri, - token as CancellationToken, - Promise.resolve('resolved') - ); - await service.cancelIndexJob(repoUri); + // create a promise and defer its fulfillment + let promiseResolve: () => void = () => {}; + const promise = new Promise(resolve => { + promiseResolve = resolve; + }); + await service.registerCancelableIndexJob(repoUri, token as CancellationToken, promise); + // do not wait on the promise, or there will be a dead lock + const cancelPromise = service.cancelIndexJob(repoUri); + // resolve the promise now + promiseResolve(); + + await cancelPromise; + + expect(cancelSpy.calledOnce).toBeTruthy(); +}); + +test('Register and cancel cancellation token while an exception is thrown from the job', async () => { + const repoUri = 'github.com/elastic/code'; + const service = new CancellationSerivce(); + const token = { + cancel: (): void => { + return; + }, + }; + const cancelSpy = sinon.spy(); + token.cancel = cancelSpy; + + // create a promise and defer its rejection + let promiseReject: () => void = () => {}; + const promise = new Promise((resolve, reject) => { + promiseReject = reject; + }); + await service.registerCancelableIndexJob(repoUri, token as CancellationToken, promise); + // expect no exceptions are thrown when cancelling the job + // do not wait on the promise, or there will be a dead lock + const cancelPromise = service.cancelIndexJob(repoUri); + // reject the promise now + promiseReject(); + + await cancelPromise; expect(cancelSpy.calledOnce).toBeTruthy(); }); diff --git a/x-pack/legacy/plugins/code/server/queue/cancellation_service.ts b/x-pack/legacy/plugins/code/server/queue/cancellation_service.ts index e83259c5eeefa..251d767ce26d7 100644 --- a/x-pack/legacy/plugins/code/server/queue/cancellation_service.ts +++ b/x-pack/legacy/plugins/code/server/queue/cancellation_service.ts @@ -68,6 +68,10 @@ export class CancellationSerivce { // Try to cancel the job first. await this.cancelJob(jobMap, repoUri); jobMap.set(repoUri, { token, jobPromise }); + // remove the record from the cancellation service when the promise is fulfilled or rejected. + jobPromise.finally(() => { + jobMap.delete(repoUri); + }); } private async cancelJob(jobMap: Map, repoUri: RepositoryUri) { @@ -77,9 +81,12 @@ export class CancellationSerivce { // 1. Use the cancellation token to pass cancel message to job token.cancel(); // 2. waiting on the actual job promise to be resolved - await jobPromise; - // 3. remove the record from the cancellation service - jobMap.delete(repoUri); + try { + await jobPromise; + } catch (e) { + // the exception from the job also indicates the job is finished, and it should be the duty of the worker for + // the job to handle it, so it's safe to just ignore the exception here + } } } } diff --git a/x-pack/legacy/plugins/code/server/routes/repository.ts b/x-pack/legacy/plugins/code/server/routes/repository.ts index c89c5b3dc2f16..88e4d1f10b904 100644 --- a/x-pack/legacy/plugins/code/server/routes/repository.ts +++ b/x-pack/legacy/plugins/code/server/routes/repository.ts @@ -9,7 +9,7 @@ import Boom from 'boom'; import { RequestFacade, ResponseToolkitFacade } from '../..'; import { validateGitUrl } from '../../common/git_url_utils'; import { RepositoryUtils } from '../../common/repository_utils'; -import { RepositoryConfig, RepositoryUri } from '../../model'; +import { RepositoryConfig, RepositoryUri, WorkerReservedProgress } from '../../model'; import { RepositoryIndexInitializer, RepositoryIndexInitializerFactory } from '../indexer'; import { Logger } from '../log'; import { RepositoryConfigController } from '../repository_config_controller'; @@ -108,10 +108,13 @@ export function repositoryRoute( // Check if the repository delete status already exists. If so, we should ignore this // request. try { - await repoObjectClient.getRepositoryDeleteStatus(repoUri); - const msg = `Repository ${repoUri} is already in delete.`; - log.info(msg); - return h.response(msg).code(304); // Not Modified + const status = await repoObjectClient.getRepositoryDeleteStatus(repoUri); + // if the delete status is an ERROR, we can give it another try + if (status.progress !== WorkerReservedProgress.ERROR) { + const msg = `Repository ${repoUri} is already in delete.`; + log.info(msg); + return h.response(msg).code(304); // Not Modified + } } catch (error) { // Do nothing here since this error is expected. log.info(`Repository ${repoUri} delete status does not exist. Go ahead with delete.`);