fix: retry on body support

[metcoder95]

This relates to...

Closes #3288

Rationale

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• Benchmarked (optional)
• Documented
• Review ready
• In review
• Merge ready

[metcoder95]

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]

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]

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

[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.

[metcoder95]

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]

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

[ronag]

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

[sdelano]

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]

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

[sdelano]

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]

@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]

@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