Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[7.x] [Code] fixed the issue that the repository can not be deleted in some cases. (#42841) #43001

Merged
merged 1 commit into from
Aug 9, 2019
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(() => {
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