fix: retry on body support #3294

metcoder95 · 2024-05-22T10:41:18Z

This relates to...

Closes #3288

Rationale

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

metcoder95 · 2024-05-22T10:42:32Z

I'd need to adjust testing and also scope properly the validation for the body; will try to do it Today's evening

ronag

isDisturbed already handles this. not sure what this is trying to fix?

metcoder95 · 2024-05-22T19:59:06Z

Yeah, the isDisturbed handles the stream correctly, but the AsyncIterator or Iterator does not behaves accordingly; we should also throw when receiving these as body as they have states and when consumed, they are not longer usable.

metcoder95 · 2024-05-24T10:40:18Z

Saw what you mean, the issue is that the handler was not wrapping the body as done for redirect handler e.g.

I made an abstraction and will open other PRs to remove the duplication and use the abstraction instead

lib/core/util.js

mcollina

I'm -1 to this implementation.

I don't think POST, PUT etc should be retried. If we want to retry them, we need to provide an on-disk store and write the content down there, so it can be resumed. The user should enable this manually.

lib/handler/retry-handler.js

metcoder95 · 2024-05-24T14:58:43Z

I'm -1 to this implementation.

I don't think POST, PUT etc should be retried. If we want to retry them, we need to provide an on-disk store and write the content down there, so it can be resumed. The user should enable this manually.

So you suggest to just remove them from the methods defaults?
That's fine by me, I can take the other way around and only retry safe-methods instead of idempotent ones.
I'd still try to abstract that class as is used across two implementations

ronag · 2024-05-24T15:07:35Z

PUT should be idempotent so I don't see why not retry.

ronag · 2024-05-24T15:17:05Z

But I don't think that has anything to do with this PR?

sdelano · 2024-05-24T17:43:01Z

I'm -1 to this implementation.

I don't think POST, PUT etc should be retried. If we want to retry them, we need to provide an on-disk store and write the content down there, so it can be resumed. The user should enable this manually.

The retry handler supports retrying network errors, and in my case (I filed the original bug) I was retrying requests that never even made it to the upstream. In my case, the connection got reset when kubernetes deployed a new set of pods (I suspect there's another undici bug here but I don't have the time to dig any deeper). If undici is going to support retrying network errors, it probably should support PUT, POST, and really any sort of HTTP method.

Additionally, not all PUTs and POSTs modify data. ElasticSearch is a very well-known example of a service that exposes database queries over HTTP POST.

It's not clear what role an on-disk store would play in these scenarios. I'm making a simple request with a string body that I want retried, but undici turned my string input into something that was internally unusable for retries.

ronag · 2024-05-24T18:00:13Z

Part of the problem is that you are using fetch? Retries are not really part of the standard.

sdelano · 2024-05-24T21:20:13Z

Part of the problem is that you are using fetch? Retries are not really part of the standard.

Okay, that's fair I guess. I'm using the node implementation of fetch, which is powered by undici, and both of these projects are under the nodejs organization in GitHub. There appears to be a tight coupling here, and I was hoping to be able to use some of the more advanced features of undici to power some libraries that are built on fetch, the standard.

I took what you said above literally, in that "this problem doesn't exist if you don't use fetch", and you're right. If I replace fetch with undici.request in my original bug report, I no longer get the same failure as before.

At this point I've worked around this issue by using fetch-retry since I can't actually use undici directly in a NextJS project as there is a bug in their compiler. Maybe at some point in the future I'll dig into why exactly undici isn't handling the closing of the connection on the other end gracefully here.

Thanks for the responses and effort here, everybody. I'll keep an eye on undici and the potential for using it more directly in the future, but for now it seems like the advanced features are better left to direct usage to use fetch in a very basic manner.

mcollina · 2024-05-25T08:56:28Z

@metcoder95 PUT POST etc are not idempotent ;).

Note that this does not fix the issue. That fix should be to not convert to a stream things that are not.

metcoder95 · 2024-05-26T09:03:20Z

@metcoder95 PUT POST etc are not idempotent ;).

Note that this does not fix the issue. That fix should be to not convert to a stream things that are not.

Oh sure, I'm not implying that POST is idempotent 😅
Rather, I was implying about that decision we made about the default methods we support to retry, which are idempotent.

As @sdelano exemplifies, the RetryHandler enables implementations to give it a try to request at their need, so even if we have defaults, they can always provide a callback that bypasses the defaults and execute retries on custom scenarios, the state assurance should be assume by the user and is something we cannot control. I'd say that that's our scope is to provide the mechanisms to ensure correctness as much as we can.

The biggest issue comes with stateful bodies (i.e. streams and iterators) that they should be backed with on-disk strategies to allow retries, which is something I'm not fully convinced to support (maybe just not yet).

The PR just attempts to identify and not retry on stateful bodies (buffers and strings can be retried "safely" up to the user).

I can also extend documentation to highlight this.

Are we ok moving this way or we prefer to take another direction?

mcollina

LGTM

lib/handler/retry-handler.js

test: add testing

c4b4e02

ronag reviewed May 22, 2024

View reviewed changes

metcoder95 added 2 commits May 22, 2024 21:59

Merge branch 'main' into fix/retry_iterator

93a0959

refactor: enhance body wrapping

38e6a6c

metcoder95 marked this pull request as ready for review May 24, 2024 10:39

Merge branch 'main' into fix/retry_iterator

d1e3516

ronag reviewed May 24, 2024

View reviewed changes

lib/core/util.js Show resolved Hide resolved

mcollina requested changes May 24, 2024

View reviewed changes

ronag requested changes May 24, 2024

View reviewed changes

lib/handler/retry-handler.js Outdated Show resolved Hide resolved

mcollina approved these changes May 27, 2024

View reviewed changes

ronag reviewed May 28, 2024

View reviewed changes

lib/handler/retry-handler.js Outdated Show resolved Hide resolved

ronag approved these changes May 28, 2024

View reviewed changes

metcoder95 added 3 commits May 29, 2024 10:15

fix: do not mutate original opts

82d7fb3

Merge branch 'main' into fix/retry_iterator

af2162e

docs: extend documentation

735a72f

ronag approved these changes May 29, 2024

View reviewed changes

ronag merged commit 5f11247 into main May 29, 2024
36 checks passed

github-actions bot mentioned this pull request May 29, 2024

[Release] v6.18.2 #3301

Merged

Uzlopak deleted the fix/retry_iterator branch June 16, 2024 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: retry on body support #3294

fix: retry on body support #3294

metcoder95 commented May 22, 2024 •

edited

metcoder95 commented May 22, 2024

ronag left a comment

metcoder95 commented May 22, 2024

metcoder95 commented May 24, 2024

mcollina left a comment

metcoder95 commented May 24, 2024

ronag commented May 24, 2024 •

edited

ronag commented May 24, 2024

sdelano commented May 24, 2024 •

edited

ronag commented May 24, 2024

sdelano commented May 24, 2024 •

edited

mcollina commented May 25, 2024

metcoder95 commented May 26, 2024

mcollina left a comment

fix: retry on body support #3294

fix: retry on body support #3294

Conversation

metcoder95 commented May 22, 2024 • edited

This relates to...

Rationale

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

metcoder95 commented May 22, 2024

ronag left a comment

Choose a reason for hiding this comment

metcoder95 commented May 22, 2024

metcoder95 commented May 24, 2024

mcollina left a comment

Choose a reason for hiding this comment

metcoder95 commented May 24, 2024

ronag commented May 24, 2024 • edited

ronag commented May 24, 2024

sdelano commented May 24, 2024 • edited

ronag commented May 24, 2024

sdelano commented May 24, 2024 • edited

mcollina commented May 25, 2024

metcoder95 commented May 26, 2024

mcollina left a comment

Choose a reason for hiding this comment

metcoder95 commented May 22, 2024 •

edited

ronag commented May 24, 2024 •

edited

sdelano commented May 24, 2024 •

edited

sdelano commented May 24, 2024 •

edited