Skip to content

HttpOverrides.runZoned doesn't appear to be working correctly #49382

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

Closed
JaffaKetchup opened this issue Jul 1, 2022 · 10 comments
Closed

HttpOverrides.runZoned doesn't appear to be working correctly #49382

JaffaKetchup opened this issue Jul 1, 2022 · 10 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-_http

Comments

@JaffaKetchup
Copy link

Hi there,

Running Dart SDK version: 2.17.3 (stable) (Wed Jun 1 11:06:41 2022 +0200) on "windows_x64"

Bit of background story first, might help. I am fixing an urgent issue in fleaflet/flutter_map#1292, which essentially involves adding the correct 'User-Agent' header to all HTTP requests generated by a NetworkImage (yes, I know this is a Flutter widget, but it doesn't matter too much I don't think).
Unfortunately, just passing the 'User-Agent' through the headers parameter doesn't work as expected. Instead of just the specified agent, both the 'Dart/2.17 (dart:io)' and custom agent are sent in what appears to be a list:
image
This is not acceptable, as we are not entirely sure how the backend checks the agent. Therefore, we need to only send the second/custom agent.

Unfortunately, I couldn't find an easy way to do this. It seems that using HttpOverrides with a custom HttpClient with the default userAgent property set to null is the best option.

Here's the snippet that doesn't appear to be working:

// The `Coords<num>` and `TileLayerOptions` parameters are irrelevant to this issue but provided for completeness

ImageProvider getImage(Coords<num> coords, TileLayerOptions options) {
  return HttpOverrides.runZoned<NetworkImage>(
    () => NetworkImage(
      getTileUrl(coords, options), // Returns a valid URL, this isn't an issue
      headers: headers, // {'User-Agent': 'flutter_map (packageName)'}
    ),
    createHttpClient: (c) {
      print('Is creating HTTP client for zone');
      return _FlutterMapHTTPOverrides().createHttpClient(c);
    },
  );
}

class _FlutterMapHTTPOverrides extends HttpOverrides {
  @override
  HttpClient createHttpClient(SecurityContext? context) {
    return super.createHttpClient(context)..userAgent = null;
  }
}

Unfortunately, with the above snippet, the createHttpClient method never seems to be run (print statement doesn't output anything), and both 'User-Agent's are sent, even though I only specified one.

When I do this, however:

ImageProvider getImage(Coords<num> coords, TileLayerOptions options) {
  HttpOverrides.global = _FlutterMapHTTPOverrides();
  return NetworkImage(
    getTileUrl(coords, options), // Returns a valid URL, this isn't an issue
    headers: headers, // {'User-Agent': 'flutter_map (packageName)'}
  );
}

It does work, as below:
image
Unfortunately, this is very far from ideal, as I don't want to be changing the root zone.

Am I doing something wrong, or is the runZoned method not working correctly? Note that I've read all of flutter/flutter#19588, and this is not a duplicate. Many thanks for any help!

@lrhn lrhn added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-_http labels Jul 2, 2022
@a-siva
Copy link
Contributor

a-siva commented Jul 6, 2022

//cc @brianquinlan

@JaffaKetchup
Copy link
Author

I need a response on this ASAP please, even if it's just a 'it will be a while until we fix', or a 'you're doing it wrong'! There is a very high-importance PR waiting on this.

@a-siva
Copy link
Contributor

a-siva commented Jul 8, 2022

I tried running a scaled down version of your code in a standalone setting

import 'dart:async';
import 'dart:io';

class MyHTTPOverrides extends HttpOverrides {
  @override
  HttpClient createHttpClient(SecurityContext? context) {
    return super.createHttpClient(context)..userAgent = null;
  }
}

void main() {
  print("Running in ${Zone.current}");
  HttpOverrides.runZoned(() {
      var httpClient = new HttpClient();
      print("userAgent = ${httpClient.userAgent}");
    },
    createHttpClient: (c) {
      print("Running in ${Zone.current}");
      return MyHTTPOverrides().createHttpClient(c);
    },
  );
}

This appears to run just fine, here is the output from this program

Running in Instance of '_RootZone'
Running in Instance of '_CustomZone'
userAgent = null

@JaffaKetchup
Copy link
Author

That's interesting. I never tried making a new client directly in the block, as it doesn't help me at all. I'll try this. If it works, I wonder if the override is being lost somehow, as the client I need to override is within another object.

@JaffaKetchup
Copy link
Author

@a-siva Hi there,

I've conducted some more research, and my findings are interesting. Indeed, if I use your snippet above, it does seem to work: the createHttpClient callback is called and used correctly.

Therefore, I edited the 'C:\Users__\Flutter\packages\flutter\lib\src\painting_network_image_io.dart' file on my system, to add some debugging print statements. I then ran my original code - with the NetworkImage - inside the runZoned. My findings are as follows:

If I add the line print(_sharedHttpClient.userAgent);, and nothing else, I receive the following result: 'Dart/2.17 (dart:io)'. This is not expected. It seems line 64 - where a HttpClient() is initialised - does not use the overridden client.

If I change line 64, to add ..userAgent = null, then the result I receive is null. This is what should happen, if I didn't change the code here, and just used the override method.

Please advise on what the issue might be, as soon as possible :).

@a-siva
Copy link
Contributor

a-siva commented Jul 8, 2022

I am not too familiar with the flutter framework code to advice on what changes need to be made in this code to allow for overrides , adding @zanderso @jonahwilliams and @dnfield to the cc list, they are more familiar with this code and could advice you.

@jonahwilliams
Copy link
Contributor

HttpOverrides needs to be applied both before the NetworkImage HTTPClient is created and in the right stack. The only place that can really be done is around runApp.

The location you've added HttpOverrides doesn't actually create the HttpClient or perform a request, that is done in a later even loop. If you want to extensively customize HttpClient creation you are better off creating your own ImageProvider

@JaffaKetchup
Copy link
Author

@jonahwilliams @a-siva

Ok, that does seem to make sense, and I should've spotted it myself! I'll have to implement a custom image provider - might have some advantages for our project anyway.

In that case, my only question remaining is, why is the original 'userAgent' header sent by the HttpClient (regardless of 'HttpOverrides'), if I specify a new one in 'headers'. As you can only really (usefully) have one of each header, why isn't the original overwritten by the new one? I might be misunderstanding something here though.

Many thanks for your help :)

JaffaKetchup added a commit to fleaflet/flutter_map that referenced this issue Jul 11, 2022
* Added `userAgentPackageName` implementation
Improved headers implementation
Deprecated `NonCachingNetworkTileProvider` in favour of `NetworkNoRetryTileProvider`
Updated example pages with changes
Improved documentation

* Temporarily fixed multiple User-Agent headers
Fixed usage of `NetworkTileProvider`
Fixed deprecation notice invalid symbol reference

* Removed some old TODOs
Simplified `_positionedForOverlay` (solved TODO)
Improved maintainability
Improved documentation

* Removed old deprecations (`placeholderImage` and `attributionBuilder`)

* Atempt to fix `HttpOverrides` issue - unsuccessful: dart-lang/sdk#49382 (comment)

* Fixed issues
Added custom `ImageProvider`s
Reduced reliance on 'universal_io' library
Prepared for review

* Fixed web platform support
Seperated tile_provider.dart for web and other plaforms
Removed 'universal_io' dependency
Updated CHANGELOG

* Fixed 'Refused to set unsafe header' error

* Improved in-code documentation

* Removed deprecated API remenant `attributionAlignment` from `TileLayerOptions`

* Fix false positive linter warning: see https://github.com/dart-lang/linter/issues/1381

* Improved documentation
Refactored base `TileProvider` into independent file
@sysint64
Copy link

For this worked:

HttpOverrides.global = _FlutterMapHTTPOverrides();

@JaffaKetchup
Copy link
Author

We were aware that this worked, but this is an unacceptable solution for a library: we shouldn't be modifying traffic outside our remit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-_http
Projects
None yet
Development

No branches or pull requests

5 participants