Skip to content

Commit

Permalink
feat: enable allowH2 by default and require Node.js >= 18.20.0 (#734)
Browse files Browse the repository at this point in the history
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

## Release Notes

- **New Features**
- Enhanced error handling across various services by introducing a
centralized timeout error checking function.
	- HTTP/2 support enabled in the HTTP client configuration.

- **Bug Fixes**
	- Corrected a typographical error in comments for better clarity.

- **Documentation**
	- Updated Node.js version requirements in the project configuration.

- **Tests**
- Improved test cases for `NpmChangesStream` and `TaskRepository` to
ensure accurate behavior and performance.

- **Chores**
	- Updated Node.js version in CI workflow for more precise testing.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
fengmk2 authored Nov 30, 2024
1 parent b808ebc commit 9b01383
Show file tree
Hide file tree
Showing 12 changed files with 77 additions and 31 deletions.
4 changes: 2 additions & 2 deletions .github/workflows/nodejs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ jobs:
strategy:
fail-fast: false
matrix:
node-version: [18, 20, 22]
node-version: [18.20.0, 18, 20, 22]
os: [ubuntu-latest]

steps:
Expand Down Expand Up @@ -83,7 +83,7 @@ jobs:
strategy:
fail-fast: false
matrix:
node-version: [18, 20, 22]
node-version: [18.20.0, 18, 20, 22]
os: [ubuntu-latest]

steps:
Expand Down
27 changes: 27 additions & 0 deletions app/common/ErrorUtil.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
const TimeoutErrorNames = [
'HttpClientRequestTimeoutError',
'HttpClientConnectTimeoutError',
'ConnectionError',
'ConnectTimeoutError',
'BodyTimeoutError',
'ResponseTimeoutError',
];

export function isTimeoutError(err: Error) {
if (TimeoutErrorNames.includes(err.name)) {
return true;
}
if (err instanceof AggregateError && err.errors) {
for (const subError of err.errors) {
if (TimeoutErrorNames.includes(subError.name)) {
return true;
}
}
}
if ('cause' in err && err.cause instanceof Error) {
if (TimeoutErrorNames.includes(err.cause.name)) {
return true;
}
}
return false;
}
10 changes: 8 additions & 2 deletions app/common/adapter/NPMRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
HttpClientResponse,
} from 'egg';
import { PackageManifestType } from '../../repository/PackageRepository';
import { isTimeoutError } from '../ErrorUtil';

type HttpMethod = HttpClientRequestOptions['method'];

Expand Down Expand Up @@ -52,9 +53,14 @@ export class NPMRegistry {
// large package: https://r.cnpmjs.org/%40procore%2Fcore-icons
// https://r.cnpmjs.org/intraactive-sdk-ui 44s
const authorization = this.genAuthorizationHeader(optionalConfig?.remoteAuthToken);
return await this.request('GET', url, undefined, { timeout: 120000, headers: { authorization } });
return await this.request('GET', url, undefined, {
timeout: 120000,
headers: { authorization },
});
} catch (err: any) {
if (err.name === 'ResponseTimeoutError') throw err;
if (isTimeoutError(err)) {
throw err;
}
lastError = err;
}
retries--;
Expand Down
2 changes: 1 addition & 1 deletion app/common/adapter/binary/AbstractBinary.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ export abstract class AbstractBinary {
for (const version of versions) {
if (!version.modules) continue;
const modulesVersion = parseInt(version.modules);
// node v6.0.0 moduels 48 min
// node v6.0.0 modules 48 min
if (modulesVersion >= 48 && !nodeABIVersions.includes(modulesVersion)) {
nodeABIVersions.push(modulesVersion);
}
Expand Down
2 changes: 1 addition & 1 deletion app/common/adapter/changesStream/NpmChangesStream.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export class NpmChangesStream extends AbstractChangeStream {
const db = this.getChangesStreamUrl(registry, since);
const { res } = await this.httpclient.request(db, {
streaming: true,
timeout: 10000,
timeout: 60000,
});

let buf = '';
Expand Down
15 changes: 7 additions & 8 deletions app/core/service/BinarySyncerService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,19 @@ import {
EggHttpClient,
} from 'egg';
import fs from 'fs/promises';
import { sortBy } from 'lodash';
import binaries, { BinaryName, CategoryName } from '../../../config/binaries';
import { NFSAdapter } from '../../common/adapter/NFSAdapter';
import { TaskType, TaskState } from '../../common/enum/Task';
import { downloadToTempfile } from '../../common/FileUtil';
import { BinaryRepository } from '../../repository/BinaryRepository';
import { Task } from '../entity/Task';
import { Binary } from '../entity/Binary';
import { TaskService } from './TaskService';
import { NFSAdapter } from '../../common/adapter/NFSAdapter';
import { downloadToTempfile } from '../../common/FileUtil';
import { isTimeoutError } from '../../common/ErrorUtil';
import { AbstractBinary, BinaryItem } from '../../common/adapter/binary/AbstractBinary';
import { AbstractService } from '../../common/AbstractService';
import { BinaryType } from '../../common/enum/Binary';
import { sortBy } from 'lodash';
import { TaskType, TaskState } from '../../common/enum/Task';

function isoNow() {
return new Date().toISOString();
Expand Down Expand Up @@ -136,12 +137,10 @@ export class BinarySyncerService extends AbstractService {
this.logger.info('[BinarySyncerService.executeTask:success] taskId: %s, targetName: %s, log: %s, hasDownloadError: %s',
task.taskId, task.targetName, logUrl, hasDownloadError);
} catch (err: any) {
task.error = err.message;
task.error = `${err.name}: ${err.message}`;
logs.push(`[${isoNow()}] ❌ Synced "${binaryName}" fail, ${task.error}, log: ${logUrl}`);
logs.push(`[${isoNow()}] ❌❌❌❌❌ "${binaryName}" ❌❌❌❌❌`);
if (err.name === 'HttpClientRequestTimeoutError'
|| err.name === 'ConnectionError'
|| err.name === 'ConnectTimeoutError') {
if (isTimeoutError(err)) {
this.logger.warn('[BinarySyncerService.executeTask:fail] taskId: %s, targetName: %s, %s',
task.taskId, task.targetName, task.error);
this.logger.warn(err);
Expand Down
19 changes: 9 additions & 10 deletions app/core/service/ChangesStreamService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,21 @@ import {
EggObjectFactory,
Inject,
} from '@eggjs/tegg';
import { TaskState, TaskType } from '../../common/enum/Task';
import { AbstractService } from '../../common/AbstractService';
import { TaskRepository } from '../../repository/TaskRepository';
import { HOST_NAME, ChangesStreamTask, Task } from '../entity/Task';
import { E500 } from 'egg-errors';
import { PackageSyncerService, RegistryNotMatchError } from './PackageSyncerService';
import { TaskService } from './TaskService';
import { RegistryManagerService } from './RegistryManagerService';
import { E500 } from 'egg-errors';
import { ScopeManagerService } from './ScopeManagerService';
import { PackageRepository } from '../../repository/PackageRepository';
import { TaskRepository } from '../../repository/TaskRepository';
import { HOST_NAME, ChangesStreamTask, Task } from '../entity/Task';
import { Registry } from '../entity/Registry';
import { AbstractChangeStream } from '../../common/adapter/changesStream/AbstractChangesStream';
import { getScopeAndName } from '../../common/PackageUtil';
import { isTimeoutError } from '../../common/ErrorUtil';
import { GLOBAL_WORKER } from '../../common/constants';
import { ScopeManagerService } from './ScopeManagerService';
import { PackageRepository } from '../../repository/PackageRepository';
import { TaskState, TaskType } from '../../common/enum/Task';
import { AbstractService } from '../../common/AbstractService';

@SingletonProto({
accessLevel: AccessLevel.PUBLIC,
Expand Down Expand Up @@ -102,9 +103,7 @@ export class ChangesStreamService extends AbstractService {
}
} catch (err) {
this.logger.warn('[ChangesStreamService.executeTask:error] %s, exit now', err.message);
if (err.name === 'HttpClientRequestTimeoutError'
|| err.name === 'ConnectTimeoutError'
|| err.name === 'BodyTimeoutError') {
if (isTimeoutError(err)) {
this.logger.warn(err);
} else {
this.logger.error(err);
Expand Down
4 changes: 2 additions & 2 deletions app/port/schedule/SyncBinaryWorker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { EggAppConfig, EggLogger } from 'egg';
import { IntervalParams, Schedule, ScheduleType } from '@eggjs/tegg/schedule';
import { Inject } from '@eggjs/tegg';
import { BinarySyncerService } from '../../core/service/BinarySyncerService';
import { isTimeoutError } from '../../common/ErrorUtil';

@Schedule<IntervalParams>({
type: ScheduleType.ALL,
Expand Down Expand Up @@ -35,8 +36,7 @@ export class SyncBinaryWorker {
const use = Date.now() - startTime;
this.logger.warn('[SyncBinaryWorker:executeTask:error] taskId: %s, targetName: %s, use %sms, error: %s',
task.taskId, task.targetName, use, err.message);
if (err.name === 'ConnectTimeoutError'
|| err.name === 'HttpClientRequestTimeoutError') {
if (isTimeoutError(err)) {
this.logger.warn(err);
} else {
this.logger.error(err);
Expand Down
1 change: 1 addition & 0 deletions config/config.default.ts
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ export default (appInfo: EggAppConfig) => {

config.httpclient = {
useHttpClientNext: true,
allowH2: true,
};

config.view = {
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,6 @@
},
"homepage": "https://github.com/cnpm/npmcore#readme",
"engines": {
"node": ">= 16.13.0"
"node": ">= 18.20.0"
}
}
18 changes: 16 additions & 2 deletions test/common/adapter/changesStream/NpmChangesStream.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ describe('test/common/adapter/changesStream/NpmChangesStream.test.ts', () => {
};
});
const res: ChangesStreamChange[] = [];
const stream = await npmChangesStream.fetchChanges(registry, '9517');
const stream = npmChangesStream.fetchChanges(registry, '9517');
for await (const change of stream) {
res.push(change);
}
Expand All @@ -74,7 +74,7 @@ describe('test/common/adapter/changesStream/NpmChangesStream.test.ts', () => {
};
});
const res: ChangesStreamChange[] = [];
const stream = await npmChangesStream.fetchChanges(registry, '9517');
const stream = npmChangesStream.fetchChanges(registry, '9517');
assert(stream);
rStream.push('{"seq":2');
rStream.push(',"id":"bac');
Expand All @@ -84,5 +84,19 @@ describe('test/common/adapter/changesStream/NpmChangesStream.test.ts', () => {
}
assert(res.length === 1);
});

it.skip('should read changes work', async () => {
for (let i = 0; i < 10000; i++) {
const stream = npmChangesStream.fetchChanges(registry, '36904024');
assert(stream);
try {
for await (const change of stream) {
console.log(change);
}
} catch (err) {
console.error(err);
}
}
});
});
});
4 changes: 2 additions & 2 deletions test/repository/TaskRepository.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,11 +106,11 @@ describe('test/repository/TaskRepository.test.ts', () => {
const newData = EntityUtil.defaultData(data, 'taskId');
const task1 = new Task(newData);
const lastSince = new Date();
await setTimeout(1);
await setTimeout(100);
task1.updatedAt = lastSince;
await taskRepository.saveTask(task1);

assert(task1.updatedAt.getTime() > lastSince.getTime());
assert(task1.updatedAt.getTime() >= lastSince.getTime());
});
});

Expand Down

0 comments on commit 9b01383

Please sign in to comment.