Skip to content

Commit

Permalink
fix: publish lock (#555)
Browse files Browse the repository at this point in the history
> When concurrently executing packet sending, there is a possibility of
version overwrite

* 🧶 Add `usingLock` logic to the publish interface to handle concurrent
execution, which will prevent version overwrite
* 🔨 Modify usingLock to include a return value indicating the success of
lock creation
------
> 并发执行发包时,可能出现版本覆盖问题
1. 🧶 在发布接口中,添加 usingLock 逻辑,包同步场景不涉及
2. 🔨 `usingLock` 添加返回值,标记是否创建锁成功
  • Loading branch information
elrrrrrrr authored Jul 21, 2023
1 parent 9ca483c commit ec90ab8
Show file tree
Hide file tree
Showing 3 changed files with 74 additions and 15 deletions.
3 changes: 2 additions & 1 deletion app/common/adapter/CacheAdapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,13 @@ export class CacheAdapter {

async usingLock(key: string, seconds: number, func: () => Promise<void>) {
const lockTimestamp = await this.lock(key, seconds);
if (!lockTimestamp) return;
if (!lockTimestamp) return false;
try {
await func();
} finally {
await this.unlock(key, lockTimestamp);
}
return true;
}

private getLockName(key: string) {
Expand Down
43 changes: 29 additions & 14 deletions app/port/controller/package/SavePackageVersionController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { isEqual } from 'lodash';
import {
UnprocessableEntityError,
ForbiddenError,
ConflictError,
} from 'egg-errors';
import {
HTTPController,
Expand All @@ -28,6 +29,7 @@ import {
} from '../../typebox';
import { RegistryManagerService } from '../../../core/service/RegistryManagerService';
import { PackageJSONType } from '../../../repository/PackageRepository';
import { CacheAdapter } from '../../../common/adapter/CacheAdapter';

const STRICT_CHECK_TARBALL_FIELDS: (keyof PackageJson)[] = [ 'name', 'version', 'scripts', 'dependencies', 'devDependencies', 'peerDependencies', 'optionalDependencies', 'license', 'licenses', 'bin' ];

Expand Down Expand Up @@ -74,6 +76,9 @@ export class SavePackageVersionController extends AbstractController {
@Inject()
private readonly registryManagerService: RegistryManagerService;

@Inject()
private readonly cacheAdapter: CacheAdapter;

// https://github.com/cnpm/cnpmjs.org/blob/master/docs/registry-api.md#publish-a-new-package
// https://github.com/npm/libnpmpublish/blob/main/publish.js#L43
@HTTPMethod({
Expand Down Expand Up @@ -199,20 +204,30 @@ export class SavePackageVersionController extends AbstractController {
}

const registry = await this.registryManagerService.ensureSelfRegistry();
const packageVersionEntity = await this.packageManagerService.publish({
scope,
name,
version: packageVersion.version,
description: packageVersion.description,
packageJson: packageVersion as PackageJSONType,
readme,
dist: {
content: tarballBytes,
},
tag: tagWithVersion.tag,
registryId: registry.registryId,
isPrivate: true,
}, user);

let packageVersionEntity;
const lockRes = await this.cacheAdapter.usingLock(`${pkg.name}:publish`, 60, async () => {
packageVersionEntity = await this.packageManagerService.publish({
scope,
name,
version: packageVersion.version,
description: packageVersion.description as string,
packageJson: packageVersion as PackageJSONType,
readme,
dist: {
content: tarballBytes,
},
tag: tagWithVersion.tag,
registryId: registry.registryId,
isPrivate: true,
}, user);
});

// lock fail
if (!lockRes) {
this.logger.warn('[package:version:add] check lock fail');
throw new ConflictError('Unable to create the publication lock, please try again later.');
}

this.logger.info('[package:version:add] %s@%s, packageVersionId: %s, tag: %s, userId: %s',
packageVersion.name, packageVersion.version, packageVersionEntity.packageVersionId,
Expand Down
43 changes: 43 additions & 0 deletions test/port/controller/package/SavePackageVersionController.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import assert from 'assert';
import { setTimeout } from 'node:timers/promises';
import { app, mock } from 'egg-mock/bootstrap';
import { TestUtil } from '../../../../test/TestUtil';
import { UserRepository } from '../../../../app/repository/UserRepository';
Expand All @@ -11,6 +12,7 @@ import { Token as TokenModel } from '../../../../app/repository/model/Token';
import { User } from '../../../../app/core/entity/User';
import dayjs from 'dayjs';
import { PackageManagerService } from '../../../../app/core/service/PackageManagerService';
import { ForbiddenError } from 'egg-errors';

describe('test/port/controller/package/SavePackageVersionController.test.ts', () => {
let userRepository: UserRepository;
Expand Down Expand Up @@ -86,6 +88,47 @@ describe('test/port/controller/package/SavePackageVersionController.test.ts', ()
assert(pkgEntity);
assert.equal(pkgEntity.registryId, selfRegistry.registryId);
});

it('should 409 when lock failed', async () => {
const { pkg, user } = await TestUtil.createPackage({ name: '@cnpm/banana', version: '1.0.0' });

const packageManagerService = await app.getEggObject(PackageManagerService);

mock(packageManagerService, 'publish', async () => {
await setTimeout(50);
throw new ForbiddenError('mock error');
});

const [ errorRes, conflictRes ] = await Promise.all([
app.httpRequest()
.put(`/${pkg.name}`)
.set('authorization', user.authorization)
.set('user-agent', user.ua)
.send(pkg),
(async () => {
await setTimeout(10);
return app.httpRequest()
.put(`/${pkg.name}`)
.set('authorization', user.authorization)
.set('user-agent', user.ua)
.send(pkg);
})(),
]);
assert(errorRes.error, '[FORBIDDEN] mock error');
assert.equal(conflictRes.status, 409);
assert(conflictRes.error, '[CONFLICT] Unable to create the publication lock, please try again later.');

// release lock
await setTimeout(50);
const nextErrorRes = await app.httpRequest()
.put(`/${pkg.name}`)
.set('authorization', user.authorization)
.set('user-agent', user.ua)
.send(pkg);
assert(nextErrorRes.error, '[FORBIDDEN] mock error');

});

it('should verify tgz and manifest', async () => {
const { pkg, user } = await TestUtil.createPackage({ name: '@cnpm/banana', version: '1.0.0' });
const pkg2 = await TestUtil.getFullPackage({ name: pkg.name, version: '0.0.1' });
Expand Down

0 comments on commit ec90ab8

Please sign in to comment.