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

native_dio_adapter does not support onReceiveProgress and onSendProgress #1835

Closed
Vovcharaa opened this issue May 26, 2023 · 13 comments
Closed
Labels
h: need extra help Extra help is needed p: dependency Targeting packages that's used by dio packages p: native_dio_adapter Targeting `native_dio_adapter` package s: feature This issue indicates a feature request

Comments

@Vovcharaa
Copy link
Contributor

Package

native_dio_adapter

Version

0.1.0

Output of flutter doctor -v

[✓] Flutter (Channel stable, 3.10.1, on macOS 13.4 22F66 darwin-arm64, locale en-UA)
    • Flutter version 3.10.1 on channel stable at /Users/vovchara/Documents/flutter
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision d3d8effc68 (10 days ago), 2023-05-16 17:59:05 -0700
    • Engine revision b4fb11214d
    • Dart version 3.0.1
    • DevTools version 2.23.1

[✓] Android toolchain - develop for Android devices (Android SDK version 33.0.0)
    • Android SDK at /Users/vovchara/Library/Android/sdk
    • Platform android-33, build-tools 33.0.0
    • Java binary at: /Applications/Android Studio.app/Contents/jre/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build 11.0.13+0-b1751.21-8125866)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 14.3)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Build 14E222b
    • CocoaPods version 1.11.3

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 2021.3)
    • Android Studio at /Applications/Android Studio.app/Contents
    • Flutter plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build 11.0.13+0-b1751.21-8125866)

[✓] VS Code (version 1.78.2)
    • VS Code at /Applications/Visual Studio Code.app/Contents
    • Flutter extension version 3.64.0

[✓] Connected device (2 available)
    • macOS (desktop) • macos  • darwin-arm64   • macOS 13.4 22F66 darwin-arm64
    • Chrome (web)    • chrome • web-javascript • Google Chrome 113.0.5672.126

[✓] Network resources
    • All expected network resources are available.

• No issues found!

Dart Version

3.0.1

Steps to Reproduce

Provide onReceiveProgress and/or onSendProgress callback to any request method.

Expected Result

Callback is called the same way as without native adapter.

Actual Result

Callback is called only once after download is done.

@Vovcharaa Vovcharaa added h: need triage This issue needs to be categorized s: bug Something isn't working labels May 26, 2023
@ueman
Copy link
Contributor

ueman commented May 31, 2023

Hey, native_dio_adapter is still experimental so some issues are to be expected.

Are you open to contribute this feature?

@ueman ueman added s: feature This issue indicates a feature request p: native_dio_adapter Targeting `native_dio_adapter` package and removed h: need triage This issue needs to be categorized s: bug Something isn't working labels May 31, 2023
@Vovcharaa
Copy link
Contributor Author

@ueman While onReceiveProgress has a pretty straightforward fix (I'll create PR today for this), I am not sure about how to fix onSendProgress because adapter converts the request object to http package request for easy integration.
There are also serious performance issues with downloading using dio_http2_adapter and onSendProgress seems to have the same issues as here (but I could not confirm it yet for sure).
Should I create a separate issue for dio_http2_adapter?

@ueman
Copy link
Contributor

ueman commented Jun 1, 2023

Awesome, thank you for the upcoming PR. Yes, please create a separate issue for dio_http2_adapter.

I am not sure about how to fix onSendProgress because adapter converts the request object to http package request for easy integration.

I was afraid of that. I think in the future we should get rid of that conversion layer, but that was the easiest way to get it working quickly. Additionally, while it's still experimental it can change at anytime, so using http as a target also kept the maintenance work to a minimum.

@Vovcharaa
Copy link
Contributor Author

I think in the future we should get rid of that conversion layer

That won't help to fix onSendProgress. cupertino_http and cronet_http drains the stream before sending the whole body to the native code. We need either to make PR with stream body implementation into their repo or create a new implementation of native clients here.

@ueman
Copy link
Contributor

ueman commented Jun 1, 2023

Oh I didn't know that. Then there should be an issue on their repo about that problem.

@Vovcharaa
Copy link
Contributor Author

I think that is desired behavior for them. They don't have any use cases for request streams, as there is no way to set the request body as a stream in http package.

ueman added a commit that referenced this issue Jun 2, 2023
<!-- Write down your pull request descriptions. -->

Part of #1835 

### New Pull Request Checklist

- [x] I have read the
[Documentation](https://pub.dev/documentation/dio/latest/)
- [x] I have searched for a similar pull request in the
[project](https://github.com/cfug/dio/pulls) and found none
- [x] I have updated this branch with the latest `main` branch to avoid
conflicts (via merge from master or rebase)
- [x] I have added the required tests to prove the fix/feature I'm
adding
- [ ] I have updated the documentation (if necessary)
- [ ] I have run the tests without failures
- [x] I have updated the `CHANGELOG.md` in the corresponding package

### Additional context and info (if any)

<!-- Provide more context and info about the PR. -->

---------

Co-authored-by: Jonas Uekötter <[email protected]>
@Vovcharaa
Copy link
Contributor Author

Vovcharaa commented Jun 30, 2023

I made a future request for cupertino_http package and when this is merged StreamingRequest will be working as intended. This should unblock the implementation of onSendProgress.
dart-lang/http#974
dart-lang/http#975

@AlexV525
Copy link
Member

dart-lang/http#974
dart-lang/http#975

They don't seem to be blocking anymore. Would you take another look if it requires further implementations? @Vovcharaa

@AlexV525 AlexV525 added p: dependency Targeting packages that's used by dio packages h: need extra help Extra help is needed labels Jul 19, 2023
@Vovcharaa
Copy link
Contributor Author

@AlexV525 It definitely requires implementation of the condition when to use StreamedRequest in native clients.
Should it be when onSendProgress callback is provided?
Or should we create a new field in RequestOptions for that?

@Vovcharaa
Copy link
Contributor Author

@AlexV525
dart-lang/http#974 (comment)
This prevents onSendProgress from working correctly.

@ueman
Copy link
Contributor

ueman commented Jul 19, 2023

That doesn't prevent implementing it in this native_dio_adapter tho. And then as soon as the cupertino_http package supports it, native_dio_adapter will too

@Vovcharaa
Copy link
Contributor Author

I agree.
The only question is when we want to use StreamedRequest because for simple requests streams could be slower in the case of native clients.

https://github.com/dart-lang/http/blob/b2067710f88980fc0fee43ec3380bce089f001db/pkgs/cupertino_http/lib/src/cupertino_client.dart#L255-L256

@AlexV525
Copy link
Member

Should it be when onSendProgress callback is provided?

I think this would be good at this moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
h: need extra help Extra help is needed p: dependency Targeting packages that's used by dio packages p: native_dio_adapter Targeting `native_dio_adapter` package s: feature This issue indicates a feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants