Skip to content

Removed clients with unrecoverable errors from the Pool #4088

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

Merged
merged 5 commits into from
Mar 12, 2025
Merged

Removed clients with unrecoverable errors from the Pool #4088

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
01a89d9
fix-3895
mcollina Mar 11, 2025
daa0ed1
added test
mcollina Mar 11, 2025
cd450c3
fixup
mcollina Mar 12, 2025
d487f7c
moved logic from pool-base to pool, as pool-base was handling this co…
mcollina Mar 12, 2025
7ce8239
lint
mcollina Mar 12, 2025
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
9 changes: 9 additions & 0 deletions lib/dispatcher/pool-base.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,15 @@ class PoolBase extends DispatcherBase {
}

this[kOnConnectionError] = (origin, targets, err) => {
// If a connection error occurs, we remove the client from the pool,
// and emit a connectionError event. They will not be re-used.
// Fixes https://github.com/nodejs/undici/issues/3895
for (const target of targets) {
// Do not use kRemoveClient here, as it will close the client,
// but the client cannot be closed in this state.
this[kClients].splice(this[kClients].indexOf(target), 1)
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible for a target not to be in the list?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, not really, but better be safe.

}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (const target of targets) {
// Do not use kRemoveClient here, as it will close the client,
// but the client cannot be closed in this state.
this[kClients].splice(this[kClients].indexOf(target), 1)
}
this[kClients] = this[kClients].filter(target => !targets.includes(target))

Here's an alternative implementation that can handle the case when target isn't in the array.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I handled it without a filter.


pool.emit('connectionError', origin, [pool, ...targets], err)
}

Expand Down
Loading