Skip to content

Conversation

@richardmtaylor
Copy link
Contributor

New attempt at cert prefetching with a completely new communication channel implementation. Now uses async callbacks rather than a background thread for the channel as this seemed to lead to very poor performance.

@richardmtaylor richardmtaylor requested review from ivolz and jexh February 18, 2025 13:54
Copy link
Contributor

@jexh jexh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it looks good.

// thread since it makes blocking calls.
// presented on any particular URL to implement the pinning.
public class ApproovHttpClientPlugin implements FlutterPlugin, MethodCallHandler {
// CertificatePrefetcher is a Runnable that fetches the certificates for a given URL. This allows the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: CertificatePrefetcher -> CertificateFetcher

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


// setup a Completer for the transaction ID we are going to use
Completer<dynamic> completer = new Completer<dynamic>();
String transactionID = ApproovService.transactionID.toString();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is thread safe in flutter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because Dart is single threaded in each isolate. Just uses co-operative scheduling across "awaits".

@richardmtaylor
Copy link
Contributor Author

So can we go ahead and merge this now?

Copy link
Contributor

@ivolz ivolz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets go.

@ivolz ivolz merged commit 16d22ff into main Feb 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants