mirror of
https://github.com/Ride-The-Lightning/RTL.git
synced 2026-08-13 12:33:07 +02:00
Address review: self-contained CLN getAlias, LND peers bound, limiter guard
Follow-up to the #1501 review (PR #1629): - F1: CLN getAlias now builds its request from selNode.authentication.options instead of the shared module-level 'options'. That coupling meant a cold Peers/route lookup dereferenced a null 'options'; with the new limiter swallowing per-task throws, that returned 200 with every alias unset. Aliases now resolve regardless of call order, with a truncated-id fallback if auth options are somehow absent. - F2: mirror the 20-way concurrency bound to LND peers (getPeers and postPeer), which had the same unbounded Promise.all alias fan-out. Eclair resolves aliases inline from a bulk nodes list, so it needs no change. - F3: normalize runWithConcurrencyLimit's start count to at least 1 so a non-positive limit can't leave 'done' unfired and hang the response.
This commit is contained in:
parent
a09eb7d4c5
commit
bd74132265
6 changed files with 50 additions and 18 deletions
|
|
@ -99,9 +99,20 @@ export const getAlias = (selNode, peer, id) => {
|
||||||
peer.alias = cached.alias;
|
peer.alias = cached.alias;
|
||||||
return Promise.resolve(peer);
|
return Promise.resolve(peer);
|
||||||
}
|
}
|
||||||
options.url = selNode.settings.lnServerUrl + '/v1/listnodes';
|
// Build a self-contained request from the selected node's own auth options rather than the
|
||||||
options.body = { id: peerId };
|
// shared module-level 'options', which is only set by a prior network.ts endpoint call. That
|
||||||
return request.post(options).then((body) => {
|
// coupling meant a cold Peers/route lookup (no prior network call) dereferenced a null 'options'
|
||||||
|
// and threw; now that the limiter swallows per-task throws, that surfaced as a 200 with every
|
||||||
|
// alias unset (#1501 review F1). selNode.authentication.options is guaranteed present here
|
||||||
|
// because every caller runs getOptions() first.
|
||||||
|
const nodeOptions = selNode.authentication?.options;
|
||||||
|
if (!nodeOptions || !nodeOptions.headers) {
|
||||||
|
peer.alias = peerId.substring(0, 20);
|
||||||
|
return Promise.resolve(peer);
|
||||||
|
}
|
||||||
|
const aliasOptions = { ...nodeOptions, method: 'POST', url: selNode.settings.lnServerUrl + '/v1/listnodes', body: { id: peerId }, json: true, qs: {} };
|
||||||
|
delete aliasOptions.form;
|
||||||
|
return request.post(aliasOptions).then((body) => {
|
||||||
logger.log({ selectedNode: selNode, level: 'DEBUG', fileName: 'Network', msg: 'Peer Alias Finished', data: body });
|
logger.log({ selectedNode: selNode, level: 'DEBUG', fileName: 'Network', msg: 'Peer Alias Finished', data: body });
|
||||||
const alias = body.nodes?.[0]?.alias || peerId.substring(0, 20);
|
const alias = body.nodes?.[0]?.alias || peerId.substring(0, 20);
|
||||||
// Re-insert so a refreshed entry moves to the most-recent position, then evict the
|
// Re-insert so a refreshed entry moves to the most-recent position, then evict the
|
||||||
|
|
|
||||||
|
|
@ -25,7 +25,10 @@ export const getPeers = (req, res, next) => {
|
||||||
request(options).then((body) => {
|
request(options).then((body) => {
|
||||||
logger.log({ selectedNode: req.session.selectedNode, level: 'DEBUG', fileName: 'Peers', msg: 'Peers List Received', data: body });
|
logger.log({ selectedNode: req.session.selectedNode, level: 'DEBUG', fileName: 'Peers', msg: 'Peers List Received', data: body });
|
||||||
const peers = !body.peers ? [] : body.peers;
|
const peers = !body.peers ? [] : body.peers;
|
||||||
return Promise.all(peers?.map((peer) => getAliasForPeers(req.session.selectedNode, peer))).then((values) => {
|
// Bound concurrent alias lookups so a node with many peers can't fire one graph/node
|
||||||
|
// request per peer at once and overwhelm the backend (parity with the CLN fix, #1501).
|
||||||
|
const getPeerAliasesTasks = peers.map((peer) => () => getAliasForPeers(req.session.selectedNode, peer));
|
||||||
|
common.runWithConcurrencyLimit(getPeerAliasesTasks, 20, () => {
|
||||||
logger.log({ selectedNode: req.session.selectedNode, level: 'INFO', fileName: 'Peers', msg: 'Sorted Peers List Received', data: body.peers });
|
logger.log({ selectedNode: req.session.selectedNode, level: 'INFO', fileName: 'Peers', msg: 'Sorted Peers List Received', data: body.peers });
|
||||||
res.status(200).json(body.peers);
|
res.status(200).json(body.peers);
|
||||||
});
|
});
|
||||||
|
|
@ -51,15 +54,14 @@ export const postPeer = (req, res, next) => {
|
||||||
options.url = req.session.selectedNode.settings.lnServerUrl + '/v1/peers';
|
options.url = req.session.selectedNode.settings.lnServerUrl + '/v1/peers';
|
||||||
request(options).then((body) => {
|
request(options).then((body) => {
|
||||||
const peers = (!body.peers) ? [] : body.peers;
|
const peers = (!body.peers) ? [] : body.peers;
|
||||||
return Promise.all(peers?.map((peer) => getAliasForPeers(req.session.selectedNode, peer))).then((values) => {
|
// Bound concurrent alias lookups (parity with the CLN fix, #1501).
|
||||||
|
const getPeerAliasesTasks = peers.map((peer) => () => getAliasForPeers(req.session.selectedNode, peer));
|
||||||
|
common.runWithConcurrencyLimit(getPeerAliasesTasks, 20, () => {
|
||||||
if (body.peers) {
|
if (body.peers) {
|
||||||
body.peers = common.newestOnTop(body.peers, 'pub_key', pubkey);
|
body.peers = common.newestOnTop(body.peers, 'pub_key', pubkey);
|
||||||
logger.log({ selectedNode: req.session.selectedNode, level: 'INFO', fileName: 'Peers', msg: 'Peers List after Connect Received', data: body });
|
logger.log({ selectedNode: req.session.selectedNode, level: 'INFO', fileName: 'Peers', msg: 'Peers List after Connect Received', data: body });
|
||||||
}
|
}
|
||||||
res.status(201).json(body.peers);
|
res.status(201).json(body.peers);
|
||||||
}).catch((errRes) => {
|
|
||||||
const err = common.handleError(errRes, 'Peers', 'Connect Peer Error', req.session.selectedNode);
|
|
||||||
return res.status(err.statusCode).json({ message: err.message, error: err.error });
|
|
||||||
});
|
});
|
||||||
}).catch((errRes) => {
|
}).catch((errRes) => {
|
||||||
const err = common.handleError(errRes, 'Peers', 'Connect Peer Error', req.session.selectedNode);
|
const err = common.handleError(errRes, 'Peers', 'Connect Peer Error', req.session.selectedNode);
|
||||||
|
|
|
||||||
|
|
@ -642,7 +642,10 @@ export class CommonService {
|
||||||
runNext();
|
runNext();
|
||||||
});
|
});
|
||||||
};
|
};
|
||||||
for (let i = 0; i < limit && i < tasks.length; i++) {
|
// Normalize to at least 1: a non-positive limit would start no tasks, so 'done' (only
|
||||||
|
// reached from a task's finally) would never fire and the response would hang.
|
||||||
|
const startCount = Math.max(1, limit);
|
||||||
|
for (let i = 0; i < startCount && i < tasks.length; i++) {
|
||||||
runNext();
|
runNext();
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
|
|
||||||
|
|
@ -100,10 +100,21 @@ export const getAlias = (selNode: SelectedNode, peer: any, id: string) => {
|
||||||
return Promise.resolve(peer);
|
return Promise.resolve(peer);
|
||||||
}
|
}
|
||||||
|
|
||||||
options.url = selNode.settings.lnServerUrl + '/v1/listnodes';
|
// Build a self-contained request from the selected node's own auth options rather than the
|
||||||
options.body = { id: peerId };
|
// shared module-level 'options', which is only set by a prior network.ts endpoint call. That
|
||||||
|
// coupling meant a cold Peers/route lookup (no prior network call) dereferenced a null 'options'
|
||||||
|
// and threw; now that the limiter swallows per-task throws, that surfaced as a 200 with every
|
||||||
|
// alias unset (#1501 review F1). selNode.authentication.options is guaranteed present here
|
||||||
|
// because every caller runs getOptions() first.
|
||||||
|
const nodeOptions = selNode.authentication?.options;
|
||||||
|
if (!nodeOptions || !nodeOptions.headers) {
|
||||||
|
peer.alias = peerId.substring(0, 20);
|
||||||
|
return Promise.resolve(peer);
|
||||||
|
}
|
||||||
|
const aliasOptions = { ...nodeOptions, method: 'POST', url: selNode.settings.lnServerUrl + '/v1/listnodes', body: { id: peerId }, json: true, qs: {} };
|
||||||
|
delete aliasOptions.form;
|
||||||
|
|
||||||
return request.post(options).then((body) => {
|
return request.post(aliasOptions).then((body) => {
|
||||||
logger.log({ selectedNode: selNode, level: 'DEBUG', fileName: 'Network', msg: 'Peer Alias Finished', data: body });
|
logger.log({ selectedNode: selNode, level: 'DEBUG', fileName: 'Network', msg: 'Peer Alias Finished', data: body });
|
||||||
const alias = body.nodes?.[0]?.alias || peerId.substring(0, 20);
|
const alias = body.nodes?.[0]?.alias || peerId.substring(0, 20);
|
||||||
// Re-insert so a refreshed entry moves to the most-recent position, then evict the
|
// Re-insert so a refreshed entry moves to the most-recent position, then evict the
|
||||||
|
|
|
||||||
|
|
@ -26,7 +26,10 @@ export const getPeers = (req, res, next) => {
|
||||||
request(options).then((body) => {
|
request(options).then((body) => {
|
||||||
logger.log({ selectedNode: req.session.selectedNode, level: 'DEBUG', fileName: 'Peers', msg: 'Peers List Received', data: body });
|
logger.log({ selectedNode: req.session.selectedNode, level: 'DEBUG', fileName: 'Peers', msg: 'Peers List Received', data: body });
|
||||||
const peers = !body.peers ? [] : body.peers;
|
const peers = !body.peers ? [] : body.peers;
|
||||||
return Promise.all(peers?.map((peer) => getAliasForPeers(req.session.selectedNode, peer))).then((values) => {
|
// Bound concurrent alias lookups so a node with many peers can't fire one graph/node
|
||||||
|
// request per peer at once and overwhelm the backend (parity with the CLN fix, #1501).
|
||||||
|
const getPeerAliasesTasks = peers.map((peer) => () => getAliasForPeers(req.session.selectedNode, peer));
|
||||||
|
common.runWithConcurrencyLimit(getPeerAliasesTasks, 20, () => {
|
||||||
logger.log({ selectedNode: req.session.selectedNode, level: 'INFO', fileName: 'Peers', msg: 'Sorted Peers List Received', data: body.peers });
|
logger.log({ selectedNode: req.session.selectedNode, level: 'INFO', fileName: 'Peers', msg: 'Sorted Peers List Received', data: body.peers });
|
||||||
res.status(200).json(body.peers);
|
res.status(200).json(body.peers);
|
||||||
});
|
});
|
||||||
|
|
@ -51,15 +54,14 @@ export const postPeer = (req, res, next) => {
|
||||||
options.url = req.session.selectedNode.settings.lnServerUrl + '/v1/peers';
|
options.url = req.session.selectedNode.settings.lnServerUrl + '/v1/peers';
|
||||||
request(options).then((body) => {
|
request(options).then((body) => {
|
||||||
const peers = (!body.peers) ? [] : body.peers;
|
const peers = (!body.peers) ? [] : body.peers;
|
||||||
return Promise.all(peers?.map((peer) => getAliasForPeers(req.session.selectedNode, peer))).then((values) => {
|
// Bound concurrent alias lookups (parity with the CLN fix, #1501).
|
||||||
|
const getPeerAliasesTasks = peers.map((peer) => () => getAliasForPeers(req.session.selectedNode, peer));
|
||||||
|
common.runWithConcurrencyLimit(getPeerAliasesTasks, 20, () => {
|
||||||
if (body.peers) {
|
if (body.peers) {
|
||||||
body.peers = common.newestOnTop(body.peers, 'pub_key', pubkey);
|
body.peers = common.newestOnTop(body.peers, 'pub_key', pubkey);
|
||||||
logger.log({ selectedNode: req.session.selectedNode, level: 'INFO', fileName: 'Peers', msg: 'Peers List after Connect Received', data: body });
|
logger.log({ selectedNode: req.session.selectedNode, level: 'INFO', fileName: 'Peers', msg: 'Peers List after Connect Received', data: body });
|
||||||
}
|
}
|
||||||
res.status(201).json(body.peers);
|
res.status(201).json(body.peers);
|
||||||
}).catch((errRes) => {
|
|
||||||
const err = common.handleError(errRes, 'Peers', 'Connect Peer Error', req.session.selectedNode);
|
|
||||||
return res.status(err.statusCode).json({ message: err.message, error: err.error });
|
|
||||||
});
|
});
|
||||||
}).catch((errRes) => {
|
}).catch((errRes) => {
|
||||||
const err = common.handleError(errRes, 'Peers', 'Connect Peer Error', req.session.selectedNode);
|
const err = common.handleError(errRes, 'Peers', 'Connect Peer Error', req.session.selectedNode);
|
||||||
|
|
|
||||||
|
|
@ -618,7 +618,10 @@ export class CommonService {
|
||||||
});
|
});
|
||||||
};
|
};
|
||||||
|
|
||||||
for (let i = 0; i < limit && i < tasks.length; i++) {
|
// Normalize to at least 1: a non-positive limit would start no tasks, so 'done' (only
|
||||||
|
// reached from a task's finally) would never fire and the response would hang.
|
||||||
|
const startCount = Math.max(1, limit);
|
||||||
|
for (let i = 0; i < startCount && i < tasks.length; i++) {
|
||||||
runNext();
|
runNext();
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue