Update image_list example to use https instead of http #39124

aam · 2019-08-23T16:29:33Z

Updated image_list test to use locally set up https server to validate performance of image loading over https(rather than http) protocol. Https requires more back-and-forward communication before image bytes are sent to the client, shows clear signs of throughput affected by how busy flutter UI isolate is.

Update image_list test to use locally set up https server to validate performance of image loading over https(rather than http) protocol. Https requires more back-and-forward communication before image bytes are sent to the client, shows clear signs of throughput affected by how busy flutter UI isolate is.

fluttergithubbot · 2019-08-23T16:29:35Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. While there are exceptions to this rule, if this patch modifies code it is probably not an exception.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

goderbauer · 2019-08-28T21:55:36Z

Why are we doing this? Is there an issue asking for this that can be linked from the PR description?
Should the NetworkImage take a SecurityContext instead of just a certificate?
How does this impact flutter for web? (cc @jonahwilliams @yjbanov)

goderbauer · 2019-08-28T21:57:35Z

Can this just be achieved with https://master-api.flutter.dev/flutter/dart-io/HttpOverrides-class.html? (e.g. Override HttpClient globally #19588 (comment))

jonahwilliams · 2019-08-28T22:00:20Z

packages/flutter/lib/src/painting/_network_image_io.dart

@@ -19,7 +19,7 @@ class NetworkImage extends image_provider.ImageProvider<image_provider.NetworkIm
  /// Creates an object that fetches the image at the given URL.
  ///
  /// The arguments [url] and [scale] must not be null.
-  const NetworkImage(this.url, { this.scale = 1.0, this.headers })
+  const NetworkImage(this.url, { this.scale = 1.0, this.headers, this.certificate })


This is unfortunately yet another constructor parameter that is not web portable

aam · 2019-08-28T22:54:50Z

Why are we doing this? Is there an issue asking for this that can be linked from the PR description?

Immediate need here is for image_list example to spin up local https(rather than http) server.
Interaction between client and server in the context of https(rather than http) session is significantly affected by how busy the client's event loop is. So https-based image_list better demonstrates "real world" impact of #34188

Should the NetworkImage take a SecurityContext instead of just a certificate?

Since this parameter needs to be sent to image loading isolate it can't be native-wrapper class, which SecurityContext is.

aam · 2019-08-28T22:58:51Z

Can this just be achieved with https://master-api.flutter.dev/flutter/dart-io/HttpOverrides-class.html? (e.g. #19588 (comment))

I think for the purposes of this test we can, I will give it a try.
But I am not sure why we would allow specifying headers with image network request, for example, but not certificate.

jonahwilliams · 2019-08-28T22:59:04Z

Should the NetworkImage take a SecurityContext instead of just a certificate?

Definitely not, because it will constrain our ability to remove dart:io code for the web, where it isn't supported.

aam · 2019-08-29T22:19:18Z

Can this just be achieved with https://master-api.flutter.dev/flutter/dart-io/HttpOverrides-class.html? (e.g. #19588 (comment))

I think for the purposes of this test we can, I will give it a try.
But I am not sure why we would allow specifying headers with image network request, for example, but not certificate.

Thanks for the suggestion @goderbauer . Created #39525 which uses HttpOverrides.global as alternative to this.

goderbauer · 2019-08-30T18:19:27Z

Can we close this PR in favor of #39525?

aam · 2019-08-30T18:38:14Z

Can this just be achieved with https://master-api.flutter.dev/flutter/dart-io/HttpOverrides-class.html? (e.g. #19588 (comment))

I think for the purposes of this test we can, I will give it a try.
But I am not sure why we would allow specifying headers with image network request, for example, but not certificate.

Thanks for the suggestion @goderbauer . Created #39525 which uses HttpOverrides.global as alternative to this.

Can we close this PR in favor of #39525?

Sorry, I should have been more careful with checking HttpOverrides behavior in presence of multiple isolates - long story short it doesn't work across isolates. :-( So even if it works for same-isolate image loading, it won't work with #39124 .

goderbauer · 2019-09-24T20:51:24Z

Since the image isolate PR got rolled back: Is this PR still relevant for the time being? Seems like we can currently achieve the same with HttpOverrides? Should we close it for now and revisit once we have decided what to do with loading images in a separate isolate?

aam · 2019-09-25T05:19:10Z

Since the image isolate PR got rolled back: Is this PR still relevant for the time being? Seems like we can currently achieve the same with HttpOverrides? Should we close it for now and revisit once we have decided what to do with loading images in a separate isolate?

Good point, I changed this PR so it is only about updating image_list example to use https instead of http.

goderbauer · 2019-09-25T16:27:27Z

examples/image_list/lib/main.dart

+  @override
+  HttpClient createHttpClient(SecurityContext context) {
+    return super.createHttpClient(
+        SecurityContext()..setTrustedCertificatesBytes(certificate.codeUnits)


Wondering: Should this modify the context that gets passed in instead of creating a new one?

goderbauer

LGTM

* Update image_list test to use locally set up https server to validate performance of image loading over https(rather than http) protocol. Https requires more back-and-forward communication before image bytes are sent to the client, shows clear signs of throughput affected by how busy flutter UI isolate is.

fluttergithubbot added d: examples Sample code and demos framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. labels Aug 23, 2019

googlebot added the cla: yes label Aug 23, 2019

aam added 2 commits August 23, 2019 09:37

Clean up analyzer warnings

21c1afa

Clean up analyzer warnings

a087f5f

aam added the a: images Loading, displaying, rendering images label Aug 23, 2019

aam mentioned this pull request Aug 23, 2019

Use separate isolate for image loading. #34188

Merged

9 tasks

jonahwilliams reviewed Aug 28, 2019

View reviewed changes

aam mentioned this pull request Aug 29, 2019

Update image_list example to use https for serving images. #39525

Closed

aam added 2 commits September 24, 2019 22:03

Merge branch 'master' into image-certificate

203873f

Use HttpOverrides instead of passing certificate through Image.network()

536f4e1

aam changed the title ~~Allow passing certificate to Image.network() for https requests.~~ Update image_list example to use https instead of http Sep 25, 2019

goderbauer reviewed Sep 25, 2019

View reviewed changes

Use provided SecurityContext if one was provided.

b738b8a

goderbauer approved these changes Sep 25, 2019

View reviewed changes

aam merged commit 4f3199f into flutter:master Sep 25, 2019

goderbauer mentioned this pull request Sep 26, 2019

Regression in image_list_jit_reported_duration #41392

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update image_list example to use https instead of http #39124

Update image_list example to use https instead of http #39124

aam commented Aug 23, 2019 •

edited

Loading

fluttergithubbot commented Aug 23, 2019

goderbauer commented Aug 28, 2019

goderbauer commented Aug 28, 2019

jonahwilliams Aug 28, 2019

aam commented Aug 28, 2019 •

edited

Loading

aam commented Aug 28, 2019

jonahwilliams commented Aug 28, 2019

aam commented Aug 29, 2019

goderbauer commented Aug 30, 2019

aam commented Aug 30, 2019 •

edited

Loading

goderbauer commented Sep 24, 2019

aam commented Sep 25, 2019

goderbauer Sep 25, 2019

aam Sep 25, 2019

goderbauer left a comment

Update image_list example to use https instead of http #39124

Update image_list example to use https instead of http #39124

Conversation

aam commented Aug 23, 2019 • edited Loading

fluttergithubbot commented Aug 23, 2019

goderbauer commented Aug 28, 2019

goderbauer commented Aug 28, 2019

jonahwilliams Aug 28, 2019

Choose a reason for hiding this comment

aam commented Aug 28, 2019 • edited Loading

aam commented Aug 28, 2019

jonahwilliams commented Aug 28, 2019

aam commented Aug 29, 2019

goderbauer commented Aug 30, 2019

aam commented Aug 30, 2019 • edited Loading

goderbauer commented Sep 24, 2019

aam commented Sep 25, 2019

goderbauer Sep 25, 2019

Choose a reason for hiding this comment

aam Sep 25, 2019

Choose a reason for hiding this comment

goderbauer left a comment

Choose a reason for hiding this comment

aam commented Aug 23, 2019 •

edited

Loading

aam commented Aug 28, 2019 •

edited

Loading

aam commented Aug 30, 2019 •

edited

Loading