feat(postgrest): abort requests#1368
Conversation
|
For now waiting for the Dart sdk bump across all packages. |
|
Now that we've already bumped the minimum versions in v2 this has not to wait for v3. |
spydon
left a comment
There was a problem hiding this comment.
Lgtm, I'll make sure that we add an example in the docs for this.
There was a problem hiding this comment.
LLM review:
Two things I'd like to sort out before merging:
-
API shape. The builder only ever reads
abortCompleter.future, it never completes it. Taking aCompleterforces callers to allocate one even when they already have a future. Accepting the trigger directly is strictly more flexible and matches howhttp'sAbortablemodels it:final response = await client.from('table').select() .abortSignal(Future.delayed(Duration(seconds: 5)));
On naming: you raised this yourself, and I'd lean towards
abortSignal(Future<void> signal). It mirrors supabase-js's.abortSignal, and aFutureis the natural Dart analog of anAbortSignal. If we change the name, let's update thesdk-compliance.yamlsymbol to match. -
The exception isn't reachable by consumers.
RequestAbortedExceptioncomes frompackage:httpand isn't re-exported bypostgrest(orsupabase), so theon RequestAbortedExceptioncatch in your example only compiles if the user addshttpas a direct dependency. Could we re-export it frompostgrest.dart, or wrap it in a postgrest-native exception?
A few minor things, none blocking:
- The empty
if/elsebranches in the newsendclosure are a bit awkward. Settingrequest.bodyonly for post/put/patch and validating unknown methods separately would read cleaner. - Bumping the shared
_buildClientretry delay to 500ms plus the mock's 200ms delay slows the whole retry suite noticeably. Could the delay be scoped to just the abort test? - The
basic_testabort test catches the exception and only prints it, relying on the elapsed-time bounds. An explicitthrowsA(isA<RequestAbortedException>())would fail louder if abort silently regressed.
Deferring functions/storage to a follow-up is fine by me.
More info:
The PR's Completer is one indirection above the http primitive. It only unwraps to .future internally anyway, so wrapping it adds nothing. Two supporting points from http's own docs:
- It explicitly blesses both patterns: Completer for event-driven abort, Future.delayed for timeouts. Accepting a Future supports both; accepting a Completer only covers the event case cleanly (a timeout caller would have to build a Completer + Timer instead of a one-liner Future.delayed).
- "This future must not complete with an error." — a contract worth mirroring in our doc comment.
…eout Makes the automatic retry behavior configurable through retryEnabled, retryCount and retryableStatusCodes on PostgrestClient and PostgrestClientOptions, and per request via PostgrestBuilder.retry (which now also accepts a count). All defaults preserve the previous behavior. Rebased on top of the request abort feature (#1368) and now builds the request timeout on top of it. A configured requestTimeout aborts the in-flight request through AbortableRequest and stops any pending retries instead of merely abandoning the future like Future.timeout would. The retry loop rethrows RequestAbortedException so an abort always stops retries, and requestTimeout and abortSignal are threaded through the reconstructing builders so they survive withConverter and count chains.
…eout Makes the automatic retry behavior configurable through retryEnabled, retryCount and retryableStatusCodes on PostgrestClient and PostgrestClientOptions, and per request via PostgrestBuilder.retry (which now also accepts a count). All defaults preserve the previous behavior. Rebased on top of the request abort feature (#1368) and now builds the request timeout on top of it. A configured requestTimeout aborts the in-flight request through AbortableRequest and stops any pending retries instead of merely abandoning the future like Future.timeout would. The retry loop rethrows RequestAbortedException so an abort always stops retries, and requestTimeout and abortSignal are threaded through the reconstructing builders so they survive withConverter and count chains.
…eout Makes the automatic retry behavior configurable through retryEnabled, retryCount and retryableStatusCodes on PostgrestClient and PostgrestClientOptions, and per request via PostgrestBuilder.retry (which now also accepts a count). All defaults preserve the previous behavior. Rebased on top of the request abort feature (#1368) and now builds the request timeout on top of it. A configured requestTimeout aborts the in-flight request through AbortableRequest and stops any pending retries instead of merely abandoning the future like Future.timeout would. The retry loop rethrows RequestAbortedException so an abort always stops retries, and requestTimeout and abortSignal are threaded through the reconstructing builders so they survive withConverter and count chains.
…eout Makes the automatic retry behavior configurable through retryEnabled, retryCount and retryableStatusCodes on PostgrestClient and PostgrestClientOptions, and per request via PostgrestBuilder.retry (which now also accepts a count). All defaults preserve the previous behavior. Rebased on top of the request abort feature (#1368) and now builds the request timeout on top of it. A configured requestTimeout aborts the in-flight request through AbortableRequest and stops any pending retries instead of merely abandoning the future like Future.timeout would. The retry loop rethrows RequestAbortedException so an abort always stops retries, and requestTimeout and abortSignal are threaded through the reconstructing builders so they survive withConverter and count chains.
…eout Makes the automatic retry behavior configurable through retryEnabled, retryCount and retryableStatusCodes on PostgrestClient and PostgrestClientOptions, and per request via PostgrestBuilder.retry (which now also accepts a count). All defaults preserve the previous behavior. Rebased on top of the request abort feature (#1368) and now builds the request timeout on top of it. A configured requestTimeout aborts the in-flight request through AbortableRequest and stops any pending retries instead of merely abandoning the future like Future.timeout would. The retry loop rethrows RequestAbortedException so an abort always stops retries, and requestTimeout and abortSignal are threaded through the reconstructing builders so they survive withConverter and count chains.
What kind of change does this PR introduce?
feature
What is the current behavior?
One cannot abort a started http request. Even by adding a
.timeoutto the feature only ignores the result of the future but does not cancel its execution.What is the new behavior?
Starting with
httpv1.5.0` it is now possible to abort requests.Supabase-js uses
.abortSignal, but since the dart implementation uses futures/completer I switched to.abortCompleter. But I'm not entirely sure and open for discussion about choosing a different name.related: #898
The issue covers the general feature to abort requests, so we can cover aborting the requests in functions in another pr.