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

Large payload with keep alive (default) causes errors #6

Closed
shurentuya opened this issue Jun 30, 2023 · 9 comments
Closed

Large payload with keep alive (default) causes errors #6

shurentuya opened this issue Jun 30, 2023 · 9 comments
Labels
documentation Improvements or additions to documentation

Comments

@shurentuya
Copy link

I'm using pocketbase realtime for web using this client as the developer mentioned here
But when I try to upload file with MultipartFile from http

await pb.collection('games').update(
        model.id,
        body: body,
        files: files,
)

It seems to break only when uploading files that are bigger than 1mb and gives me this error

ClientException: {url: https://url/api/collections/test/records/3gc4a6fn6hgi4nh, isAbort: true, statusCode: 0, response: {}, originalError: Failed to execute fetch: TypeError: Failed to fetch}
dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 288:49  throw_
packages/pocketbase/src/client.dart 200:9                                     send
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 60:31            <fn>
dart-sdk/lib/async/zone.dart 1666:54                                          runBinary
dart-sdk/lib/async/future_impl.dart 162:22                                    handleError
dart-sdk/lib/async/future_impl.dart 796:46                                    handleError
dart-sdk/lib/async/future_impl.dart 817:13                                    _propagateToListeners
dart-sdk/lib/async/future_impl.dart 592:5                                     [_completeError]
dart-sdk/lib/async/future_impl.dart 683:7                                     callback
dart-sdk/lib/async/schedule_microtask.dart 40:11                              _microtaskLoop
dart-sdk/lib/async/schedule_microtask.dart 49:5                               _startMicrotaskLoop
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 177:15           <fn>
@Zekfad
Copy link
Owner

Zekfad commented Jun 30, 2023

This probably related to CORS, try this: https://stackoverflow.com/questions/49343024/getting-typeerror-failed-to-fetch-when-the-request-hasnt-actually-failed
TypeError: Failed to fetch is exception thrown by browser (not module), meaning that it can't get valid data to satisfy fetch's Promise.
If this doesn't help, could you provide snapshot of XHR that failed (from devtools), If you can, please provide instructions how to re-create your environment, so I could debug it.?

@ganigeorgiev
Copy link

ganigeorgiev commented Jul 1, 2023

@Zekfad I don't think is CORS related because with smaller files it seems to work fine.

Below is a minimal setup using webdev serve:

  1. example/pubspec.yaml

    name: example
    version: 0.0.1
    
    environment:
      sdk: '>=2.12.0 <4.0.0'
    
    dependencies:
      fetch_client: ^1.0.0
    
    dev_dependencies:
      build_runner: '>=1.6.2 <3.0.0'
      build_web_compilers: '>=2.12.0 <4.0.0'
  2. example/index.html

    <!DOCTYPE html>
    <html>
      <head>
        <script defer src="example.dart.js"></script>
      </head>
      <body>
        <input type="number" id="sizeInput" placeholder="Total bytes" value="100000">
        <button id="uploadBtn">Upload</button>
      </body>
    </html>
  3. example/example.dart

    import 'dart:html';
    import 'dart:typed_data';
    import 'package:http/http.dart' as http;
    import 'package:fetch_client/fetch_client.dart';
    
    void main() {
      // random request bin url
      // (could be inspected at https://public.requestbin.com/r/en80s0kpkubki)
      final url = Uri.parse('https://en80s0kpkubki.x.pipedream.net');
      final sizeInput = document.getElementById('sizeInput') as InputElement;
      final uploadBtn = document.getElementById('uploadBtn');
    
      uploadBtn?.onClick.listen((event) {
        final size = int.parse(sizeInput.value ?? '0');
        final file = http.MultipartFile.fromBytes('test', Uint8List(size));
        final req = http.MultipartRequest('POST', url)..files.add(file);
    
        print('Uploading file with $size bytes...');
    
        FetchClient(mode: RequestMode.cors).send(req).then((result) {
          print('SUCCESS!');
        }).catchError((err) {
          print('ERROR: $err');
        });
      });
    }
  4. Run dart pub global run webdev serve example

A test with 10000 bytes works fine, but with 100000 throws an error while the request remains in a "pending" state.

@Zekfad
Copy link
Owner

Zekfad commented Jul 1, 2023

This took me some time to figure out, but it seems that fetch with keepalive: true and large enough payload breaks.
I guess this is to prevent bandwidth leak when losing tab (keepalive can outlive closed page).

To prevent fetch from setting keepalive, set persistentConnection to false in BaseRequest.
For your example:

final req = http.MultipartRequest('POST', url)
  ..files.add(file)
  ..persistentConnection = false;

Regarding "(pending)" in network tab, this seems like a quirk of chrome, because request is never sent and fetch never executed, but throwed instead. This means request isn't stuck or leaked, it just never happened, so you can safely ignore this and process with .catchError or try-catch.

@ganigeorgiev
Copy link

Thank you for looking at the issue. That's certainly strange because it is working fine when using directly the browsers fetch which I believe also uses true as default (maybe there is some obscure implementation detail with the dart js interop, I'm not sure).

@shurentuya Before updating it in the PocketBase Dart SDK, I'll have to experiment whether the disabled persistentConnection option will not break something else with the default http client for other users.

@Zekfad
Copy link
Owner

Zekfad commented Jul 1, 2023

Looking at your module, I suggest adding check for size and persistentConnection = false to following part:
https://github.com/pocketbase/dart-sdk/blob/9e2f7d6c980cc1a446ebfc55fc957501f64ff198/lib/src/client.dart#L168

Keep-alive allows you to reuse connection for further requests, but it looks like it has it's own limitations, here are spec:

https://fetch.spec.whatwg.org/#http-network-or-cache-fetch see 4.5. list: 8.10.5:

If the sum of contentLength and inflightKeepaliveBytes is greater than 64 kibibytes, then return a network error.

@Zekfad
Copy link
Owner

Zekfad commented Jul 1, 2023

it is working fine when using directly the browsers fetch which I believe also uses true as default (maybe there is some obscure implementation detail with the dart js interop, I'm not sure).

Yeah, I've check it, it seems Chrome have some mechanism for that (my assumption - flag is implicitly true unless payload is large than 65KB).
Spec says that keepalive is false by default:
https://fetch.spec.whatwg.org/#request-keepalive-flag

@ganigeorgiev
Copy link

ganigeorgiev commented Jul 1, 2023

Hm, this seems aligned with the spec https://fetch.spec.whatwg.org/#http-network-or-cache-fetch:

  1. If contentLength is non-null and httpRequest’s keepalive is true, then:
    ...
    If the sum of contentLength and inflightKeepaliveBytes is greater than 64 kibibytes, then return a network error.

But I'm still not sure why it is working fine with the plain js fetch and FormData object.

edit: It seems that we posted almost at the same time. I'll experiment with disabling the flag.

@Zekfad
Copy link
Owner

Zekfad commented Jul 1, 2023

Set keepalive to true and it will also falter.
You may notice that correct request doesn't have Connection: Keep-Alive header.

I've also checked that you have limitation regarding request cancel, this can be done via fetch, if you don't mind depending on it.

@Zekfad Zekfad changed the title Using it with pocketbase to use it in flutter web for realtime Large payload with keep alive (default) causes errors Jul 1, 2023
@ganigeorgiev
Copy link

@shurentuya Just to let you know that I've pushed a fix in PocketBase SDK v0.10.3 and the fetch_client request should no longer result in the above TypeError (I've decided to always disable the keepalive for web since the content length may not be always available to perform the check and the behavior is the same anyway with the default http.Client because the persistentConnection option is ignored on web; eventually in the future I'll consider adding an option to allow changing it per request basis in case a custom route or middleware is used).

Anyway, thank you @Zekfad for debugging the issue!

@Zekfad Zekfad closed this as completed in 0112b99 Jul 1, 2023
@Zekfad Zekfad added the documentation Improvements or additions to documentation label Jul 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

3 participants