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

ok_http: Add BaseClient Implementation and make asynchronous requests. #1215

Merged
merged 15 commits into from
May 31, 2024

Conversation

Anikate-De
Copy link
Contributor

Items in this PR:

  • Generated necessary JNI Bindings for BaseClient implementation
  • Allow asynchronous requests using the enqueue() API
  • Reuse the Books example from pkgs/cronet_http/example/
  • Setup Integration Tests
  • Add minimal documentation
  • Update CHANGELOG.md: 0.1.0-wip entry
  • Add testing workflow

  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@github-actions github-actions bot added the type-infra A repository infrastructure change or enhancement label May 29, 2024
@brianquinlan brianquinlan self-requested a review May 29, 2024 22:30
@brianquinlan
Copy link
Collaborator

Amazing job!

You might need to run dart format to fix the analysis errors.

@Anikate-De
Copy link
Contributor Author

Anikate-De commented May 30, 2024

@brianquinlan Thank you!

I'll go ahead and do that.

There seems to be a problem with the test setup as it always force terminates while executing test multiple clients multiple clients with simultaneous requests. I tried with a larger timeout of 360s, but no luck :(

Upon further testing, I believe java.net.SocketTimeoutException is the culprit as OkHttp has a default timeout of 10s. I'll try increasing this and see if it works. Do you have any suggestions for this?

@Anikate-De
Copy link
Contributor Author

SocketTimeoutException was all good, I managed to increase the timeout by generating the bindings of java.util.concurrent.TimeUnit. I propose that we make the timeout configurable for this package. I'll hold that off for later PRs.

As for the test, I traced the issue back to how the response is being sent by the test server. Things that fix it:

  1. Add a 1 second delay anywhere inside onResponse
await Future.delayed(const Duration(seconds: 1));
  1. OR, Don't send i and remove test expect for response body (defeats the purpose of the test)
  responseFutures.add(client.get(Uri.http(host, '')));
}
final responses = await Future.wait(responseFutures);
  for (var i = 0; i < 5; ++i) {
    expect(responses[i].statusCode, 200);
    // expect(responses[i].body, i.toString());
  }

(1.) shows that the enqueue API of OkHttp is unable to return outputs in the same thread when it is forced to wait for the server to add a response.
I believe that this will be fixed once we implement functionality to Stream Responses.

I'm making some assumptions here, so hopefully this analysis is not wrong.

pkgs/ok_http/jnigen.yaml Show resolved Hide resolved
pkgs/ok_http/jnigen.yaml Show resolved Hide resolved
pkgs/ok_http/lib/ok_http.dart Outdated Show resolved Hide resolved
pkgs/ok_http/lib/src/ok_http_client.dart Outdated Show resolved Hide resolved
pkgs/ok_http/lib/src/ok_http_client.dart Outdated Show resolved Hide resolved
pkgs/ok_http/lib/src/ok_http_client.dart Show resolved Hide resolved
pkgs/ok_http/lib/src/ok_http_client.dart Outdated Show resolved Hide resolved
pkgs/ok_http/lib/src/ok_http_client.dart Show resolved Hide resolved
pkgs/ok_http/lib/src/ok_http_client.dart Outdated Show resolved Hide resolved
fixed comments
run individual tests that pass

Co-authored-by: Brian Quinlan <brian@sweetapp.com>
encapsulate only responseBody reading in try block and mark as blocking call

Co-authored-by: Brian Quinlan <brian@sweetapp.com>
Copy link
Collaborator

@brianquinlan brianquinlan left a comment

Choose a reason for hiding this comment

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

Nice!

@Anikate-De
Copy link
Contributor Author

Thanks a lot! @brianquinlan

@brianquinlan brianquinlan merged commit 7bfbeea into dart-lang:master May 31, 2024
28 of 29 checks passed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Jun 3, 2024
…ctor, browser_launcher, cli_util, clock, collection, convert, crypto, csslib, dartdoc, ecosystem, file, fixnum, glob, html, http, http_multi_server, http_parser, json_rpc_2, lints, logging, markdown, matcher, mime, package_config, path, pool, pub_semver, shelf, source_map_stack_trace, source_maps, source_span, sse, stack_trace, stream_channel, string_scanner, sync_http, term_glyph, test_descriptor, test_process, test_reflective_loader, typed_data, vector_math, watcher, yaml

Revisions updated by `dart tools/rev_sdk_deps.dart`.

args (https://github.com/dart-lang/args/compare/b3608bd..6a5a2e6):
  6a5a2e6  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.4 to 4.1.6 in the github-actions group (dart-archive/args#276)

async (https://github.com/dart-lang/async/compare/f933ddf..4073129):
  4073129  2024-06-02  dependabot[bot]  Bump actions/checkout from 4.1.4 to 4.1.6 in the github-actions group (dart-archive/async#276)

bazel_worker (https://github.com/dart-lang/bazel_worker/compare/2ec90c0..c76d7c8):
  c76d7c8  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.4 to 4.1.6 in the github-actions group (dart-archive/bazel_worker#93)

benchmark_harness (https://github.com/dart-lang/benchmark_harness/compare/6fa42b0..f6ef33d):
  f6ef33d  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.4 to 4.1.6 in the github-actions group (dart-archive/benchmark_harness#107)

boolean_selector (https://github.com/dart-lang/boolean_selector/compare/84467e5..62f82f6):
  62f82f6  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.4 to 4.1.6 in the github-actions group (dart-archive/boolean_selector#62)

browser_launcher (https://github.com/dart-lang/browser_launcher/compare/0dcf224..7348cea):
  7348cea  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.4 to 4.1.6 in the github-actions group (dart-archive/browser_launcher#60)

cli_util (https://github.com/dart-lang/cli_util/compare/9fe3eeb..c37d5e1):
  c37d5e1  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.4 to 4.1.6 in the github-actions group (dart-archive/cli_util#104)

clock (https://github.com/dart-lang/clock/compare/80e70ac..7cbf08e):
  7cbf08e  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.4 to 4.1.6 in the github-actions group (dart-archive/clock#65)

collection (https://github.com/dart-lang/collection/compare/fe2e800..586a5e8):
  586a5e8  2024-06-02  dependabot[bot]  Bump actions/checkout from 4.1.4 to 4.1.6 in the github-actions group (dart-archive/collection#346)

convert (https://github.com/dart-lang/convert/compare/056626e..302af1b):
  302af1b  2024-06-02  dependabot[bot]  Bump actions/checkout from 4.1.4 to 4.1.6 in the github-actions group (dart-archive/convert#106)

crypto (https://github.com/dart-lang/crypto/compare/3f815ac..0a10171):
  0a10171  2024-06-02  dependabot[bot]  Bump actions/checkout from 4.1.4 to 4.1.6 in the github-actions group (dart-archive/crypto#171)

csslib (https://github.com/dart-lang/csslib/compare/141dd65..23c314b):
  23c314b  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.4 to 4.1.6 in the github-actions group (dart-archive/csslib#202)

dartdoc (https://github.com/dart-lang/dartdoc/compare/ed97585..45627f9):
  45627f92  2024-06-01  dependabot[bot]  Bump the github-actions group across 1 directory with 3 updates (dart-lang/dartdoc#3777)

ecosystem (https://github.com/dart-lang/ecosystem/compare/ad9da15..bc25c0c):
  bc25c0c  2024-06-01  dependabot[bot]  Bump the dependencies group with 4 updates (dart-lang/ecosystem#265)

file (https://github.com/google/file.dart/compare/8ce0d13..07cacae):
  07cacae  2024-06-03  dependabot[bot]  Bump actions/checkout from 4.1.4 to 4.1.6 (dart-archive/file.dart#240)

fixnum (https://github.com/dart-lang/fixnum/compare/ac892ad..a8157d8):
  a8157d8  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.4 to 4.1.6 in the github-actions group (dart-archive/fixnum#130)

glob (https://github.com/dart-lang/glob/compare/ee48ea8..eaec6a4):
  eaec6a4  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-lang/glob#94)

html (https://github.com/dart-lang/html/compare/00d3461..3bc803d):
  3bc803d  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-archive/html#247)

http (https://github.com/dart-lang/http/compare/6337ee3..7bfbeea):
  7bfbeea  2024-06-01  Anikate De  `ok_http`: Add BaseClient Implementation and make asynchronous requests. (dart-lang/http#1215)

http_multi_server (https://github.com/dart-lang/http_multi_server/compare/4a791af..25941e2):
  25941e2  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-lang/http_multi_server#70)

http_parser (https://github.com/dart-lang/http_parser/compare/702698a..551e0e4):
  551e0e4  2024-06-02  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-archive/http_parser#93)

json_rpc_2 (https://github.com/dart-lang/json_rpc_2/compare/3187f7b..5b1cbd6):
  5b1cbd6  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-archive/json_rpc_2#115)

lints (https://github.com/dart-lang/lints/compare/b254c7e..baaaa56):
  baaaa56  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.4 to 4.1.6 (dart-lang/lints#191)

logging (https://github.com/dart-lang/logging/compare/7f722dc..73f043a):
  73f043a  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.4 to 4.1.6 (dart-archive/logging#166)

markdown (https://github.com/dart-lang/markdown/compare/c1013dc..3d8d7a8):
  3d8d7a8  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-lang/markdown#614)

matcher (https://github.com/dart-lang/matcher/compare/4ac4096..0abd405):
  0abd405  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-archive/matcher#249)

mime (https://github.com/dart-lang/mime/compare/b01c9a2..8d2d559):
  8d2d559  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.4 to 4.1.6 (dart-archive/mime#124)

package_config (https://github.com/dart-lang/package_config/compare/3909676..903a0e5):
  903a0e5  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-lang/package_config#155)

path (https://github.com/dart-lang/path/compare/aea50fa..8fc4c72):
  8fc4c72  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-archive/path#167)

pool (https://github.com/dart-lang/pool/compare/1a6f2df..88e4636):
  88e4636  2024-06-02  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-lang/pool#87)

pub_semver (https://github.com/dart-lang/pub_semver/compare/f57c9c3..a9025f3):
  a9025f3  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-lang/pub_semver#104)

shelf (https://github.com/dart-lang/shelf/compare/338962c..ea3c983):
  ea3c983  2024-06-02  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-lang/shelf#434)

source_map_stack_trace (https://github.com/dart-lang/source_map_stack_trace/compare/6834af5..96a8213):
  96a8213  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-archive/source_map_stack_trace#55)

source_maps (https://github.com/dart-lang/source_maps/compare/181a41c..caa79c2):
  caa79c2  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-lang/source_maps#94)

source_span (https://github.com/dart-lang/source_span/compare/e80cb44..59a3903):
  59a3903  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-lang/source_span#113)

sse (https://github.com/dart-lang/sse/compare/1bb0a98..7dcde16):
  7dcde16  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-lang/sse#111)

stack_trace (https://github.com/dart-lang/stack_trace/compare/4d346f7..ab09060):
  ab09060  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-lang/stack_trace#155)

stream_channel (https://github.com/dart-lang/stream_channel/compare/61ad872..b41ff7a):
  b41ff7a  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-lang/stream_channel#107)

string_scanner (https://github.com/dart-lang/string_scanner/compare/32468bd..7b37c1b):
  7b37c1b  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-lang/string_scanner#75)

sync_http (https://github.com/dart-lang/sync_http/compare/82553db..7622bdd):
  7622bdd  2024-06-03  dependabot[bot]  Bump actions/checkout from 4.1.4 to 4.1.6 (google/sync_http.dart#47)

term_glyph (https://github.com/dart-lang/term_glyph/compare/a46b48b..c86e817):
  c86e817  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-lang/term_glyph#53)

test_descriptor (https://github.com/dart-lang/test_descriptor/compare/d61bf6c..b23d7cc):
  b23d7cc  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-lang/test_descriptor#67)

test_process (https://github.com/dart-lang/test_process/compare/4ab3f1c..862eaf3):
  862eaf3  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-lang/test_process#59)

test_reflective_loader (https://github.com/dart-lang/test_reflective_loader/compare/f8807e0..816942e):
  816942e  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-lang/test_reflective_loader#63)

typed_data (https://github.com/dart-lang/typed_data/compare/fb1958c..d14f965):
  d14f965  2024-06-02  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-archive/typed_data#88)

vector_math (https://github.com/google/vector_math.dart/compare/43f2a77..3c03ac3):
  3c03ac3  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.2 to 4.1.6 (google/vector_math.dart#325)

watcher (https://github.com/dart-lang/watcher/compare/c182cd3..c00fc2a):
  c00fc2a  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-lang/watcher#168)

yaml (https://github.com/dart-lang/yaml/compare/8fb8147..7873b3f):
  7873b3f  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-lang/yaml#165)

Change-Id: I734de0d6c7013c49b879f927d5ecda4d675b19ce
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/369520
Commit-Queue: Devon Carew <devoncarew@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-infra A repository infrastructure change or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants