fix: returning values instead of pointers from cronet #7
Conversation
unsuitable001
changed the title
|
@dcharkes @mannprerak2 Should I replicate the buffer too before sending it to the dart side? Currently, buffer pointer is getting passed and I haven't faced any issue yet, even in benchmarking. |
| @@ -20,6 +21,11 @@ Cronet_RESULT (*_Cronet_Engine_Shutdown)(Cronet_EnginePtr self); | |||
| void (*_Cronet_Engine_Destroy)(Cronet_EnginePtr self); | |||
| Cronet_BufferPtr (*_Cronet_Buffer_Create)(void); | |||
| void (*_Cronet_Buffer_InitWithAlloc)(Cronet_BufferPtr self, uint64_t size); | |||
| int32_t (*_Cronet_UrlResponseInfo_http_status_code_get)( | |||
There was a problem hiding this comment.
We need two integers for major and minor revision in this wrapper.
That way we can look those up in the Dart code and throw an exception that the wrapper needs to be recompiled/redownloaded if people update the package but don't run the script again. (Instead of crashing with symbol not found.)
There was a problem hiding this comment.
Done. Marked as 1.0. Bumping major version to 1 as a breaking change is introduced.
Breaking API: InitCronetApi
maked this patch as 1.0. Incrementing major version due to breaking changes.
|
Added tests for redirects and exposed set methods for
Version bumped to 0.0.1+1 |
| import 'dylib_handler.dart'; | ||
| import 'third_party/cronet/generated_bindings.dart'; | ||
| import 'wrapper/generated_bindings.dart'; | ||
|
|
||
| Wrapper loadAndInitWrapper() { | ||
| final wrapper = Wrapper(loadWrapper()); | ||
| if (wrapperVersion != wrapper.VersionString().cast<Utf8>().toDartString()) { |
There was a problem hiding this comment.
If we're doing just a comparison, there is no reason to have a major and minor revision. Then we can just have a simple number.
In some cases an API gets extra functionality, in those cases you only bump the minor revision. And you can keep using the previous binary if you're never calling the new functionality with dart:ffi. But if this wrapper is set up in a way that we're most likely always have to do a major version increase, we might as well just have only major version.
There was a problem hiding this comment.
Keeping it for version only. Let's plan to move to major.minor format when the wrapper is little more stable.
We shouldn't return pointers that we received as a callback parameter from cronet. In a slower system or while doing bunch of requests togather (e.g. benchmarking), we can accidentally dereference those cronet created pointers after cronet frees them.
UTF8 decoding the newLocation URL string which is received as a callback of onRedirect event causes the following error:
This PR fixes this issue by dereferencing and getting the raw data from cronet created pointers in the wrapper then sending copied data to the dart side via nativeport.
Also patched crash on parallel request if 404 is received.
NOTE: Update Binaries.