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.

Sign up for GitHub

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

[Code] fixed the issue that the repository can not be deleted in some cases. #42841

Merged
merged 3 commits into from
Aug 9, 2019
Merged

[Code] fixed the issue that the repository can not be deleted in some cases. #42841

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
764a558
[Code] fixed the issue that the repository can not be deleted in some
Aug 7, 2019
e8226ad
[Code] catch the rejected exception while cancelling the job, and added
Aug 8, 2019
5461326
[Code] refined tests for CancellationService by deferred completion of
Aug 8, 2019
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
10 changes: 5 additions & 5 deletions x-pack/legacy/plugins/code/server/queue/cancellation_service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,11 @@ test('Register and cancel cancellation token', async () => {
const cancelSpy = sinon.spy();
token.cancel = cancelSpy;

await service.registerCancelableIndexJob(
repoUri,
token as CancellationToken,
Promise.resolve('resolved')
);
// make sure the promise won't be fulfilled immediately
const promise = new Promise(resolve => {
setTimeout(resolve, 100);
Copy link
Contributor

Choose a reason for hiding this comment

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

the best way to manipulate the timer in unit test is using sinon's fake timer (example here). Otherwise, the test will have some risks of flaky. It's up to you if you want to update it.

Copy link
Author

Choose a reason for hiding this comment

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

The intention here is to make sure the promise is completed after service.cancelIndexJob(repoUri) is called, otherwise the promise will be removed from the map. I tried another way to defer its resolution and rejection, as introduced in this question. It seems a bit not clean here, any better ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

there are some other options

  1. use Promise.deferred, basically it's same as your current approach but the API is obsolete
  2. use third party library like https://www.npmjs.com/package/delay, which is basically your previous approach

so overall, I think your current solution is good enough

Copy link
Contributor

@mw-ding mw-ding Aug 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@fantapsody My point here is not the way you defer the promise wrong. I should be clear here. Even if you defer 100ms here, there are still some chances that the promise resolves before the cancelIndexJob. A better way is to create a faketimer and then tick only 50ms to guarantee this won't happen.

await service.registerCancelableIndexJob(repoUri, token as CancellationToken, promise);

// Tick 50ms so it's 100% sure that the promise won't complete before the next statement
faketimer.tick(50)

await service.cancelIndexJob(repoUri);

Again, it's up to you if you want to make the change or not. no strong opinion here. Just introduce you this powerful faketimer tool since you will probably need this sooner or later.

});
await service.registerCancelableIndexJob(repoUri, token as CancellationToken, promise);
fantapsody marked this conversation as resolved.
Show resolved Hide resolved
await service.cancelIndexJob(repoUri);

expect(cancelSpy.calledOnce).toBeTruthy();
Expand Down
7 changes: 5 additions & 2 deletions x-pack/legacy/plugins/code/server/queue/cancellation_service.ts
View file
Open in desktop
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,8 @@ 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
// if jobPromise is rejected, is it safer to just throw the exception and let the caller retry?
fantapsody marked this conversation as resolved.
Show resolved Hide resolved
await jobPromise;
// 3. remove the record from the cancellation service
jobMap.delete(repoUri);
}
}
}
13 changes: 8 additions & 5 deletions x-pack/legacy/plugins/code/server/routes/repository.ts
View file
Open in desktop
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