Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
13 changes: 10 additions & 3 deletions x-pack/legacy/plugins/code/server/queue/cancellation_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is indeed better to remove the job from the service map. thanks.

jobMap.delete(repoUri);
});
}

private async cancelJob(jobMap: Map<RepositoryUri, CancellableJob>, repoUri: RepositoryUri) {
Expand All @@ -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
}
}
}
}
13 changes: 8 additions & 5 deletions x-pack/legacy/plugins/code/server/routes/repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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.`);
Expand Down