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.

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

fix: call await body.text() to avoid undici SocketError: other side closed error #337

Closed
wants to merge 1 commit into from

Conversation

digitalsadhu
Copy link
Member

@digitalsadhu digitalsadhu commented Dec 1, 2023

This PR solves the following error that occurs intermittently (but frequently).

[14:09:16.152] [FATAL] - global uncaught exception - terminating with error. - {
  value: 'SocketError: other side closed\n' +
    '    at Socket.onSocketEnd (/Users/richard.walker@finn.no/src/frontpage-podium/node_modules/undici/lib/client.js:1129:22)\n' +
    '    at Socket.emit (node:events:526:35)\n' +
    '    at endReadableNT (node:internal/streams/readable:1589:12)\n' +
    '    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)'
} - SocketError: other side closed
    at Socket.onSocketEnd (/Users/richard.walker@finn.no/src/frontpage-podium/node_modules/undici/lib/client.js:1129:22)
    at Socket.emit (node:events:526:35)
    at endReadableNT (node:internal/streams/readable:1589:12)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)
[14:09:16.158] [ERROR] - Exiting with status 1 in 2 seconds.

There is a massive Undici issue thread thats related nodejs/undici#583 with an unsatisfactory result that concludes that you need to call await body.text(); to avoid the issue which is what this PR does at the expense of essentially buffering up the fetch body before chunking it up again and streaming it as usual.

This is one (sub optimal) way to fix the issue but it does seem reliable. Perhaps there are better ways to fix it? @trygve-lie

Copy link
Contributor

@wkillerud wkillerud left a comment

Choose a reason for hiding this comment

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

😿

@leftieFriele
Copy link
Contributor

should we add tests to verify it works? it might be useful if we want to apply a different fix later.

@digitalsadhu
Copy link
Member Author

I think we’ll see what @trygve-lie has to say. This fix is pretty non optimal as it buffers up the stream before chunking it up again which defeats the purpose of the streaming in the first place. More tests is good though of course. If on can be written for an intermittent bug like this.

@trygve-lie
Copy link
Contributor

See: #338.

The real problem seems to be that this only errors when a Podlet responds with a http error. Iow; the way we consume Podlets when they are healthy works as intended but when an Podlet errors we are not consuming the body which Undici require us to do.

This PR works because it consumes the body before we check for http errors but it have the side effect that it also degrades how we consume healthy Podlets.

#388 is a more fine grained in consuming the body.

@trygve-lie
Copy link
Contributor

Fixed in #338.

@trygve-lie trygve-lie closed this Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants