pkgs/ok_http: Close and remove all idle connections from the resource pool on response

[Anikate-De]

Items in this PR:

• Generated JNI Bindings for okhttp3.ConnectionPool (allows us to use the evictAll() method)
• Close all idle connections in the resource pool onResponse()
  • WHY?: During testRequestCookies(), the server determines the requests only by binding on the socket. And, OkHttp never frees or closes connections for all enqueue() calls. This led to the server assuming multiple requests as a single entity (as OkHttp never stopped communicating with the socket,) and parsing both cookie headers as one. This gave a cookies list with a length of 2, which failed the 2nd test case multiple cookies semicolon separated. By explicitly closing idle connections, we protect consecutive requests from being overlapped.
• Add testRequestCookies()
• Add testResponseHeaders() since all TCs are passing now that [http_client_conformance_tests] Add flag to skip tests for folded headers #1219 is closed (see pkgs/http_client_conformance_tests: add boolean flag supportsFoldedHeaders #1222)

---

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

Contribution guidelines:

• See our contributor guide for general expectations for PRs.
• Larger or significant changes should be discussed in an issue before creating a PR.
• Contributions to our repos should follow the Dart style guide and use dart format.
• Most changes should add an entry to the changelog and may need to rev the pubspec package version.
• Changes to packages require corresponding tests.

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

[brianquinlan]

I read your explanation but I still don't understand why this is necessary. Since the socket is closed by the server after receiving the headers, how can it be reused?

http/pkgs/http_client_conformance_tests/lib/src/request_cookies_server.dart

Line 48 in 9f022d2

await socket.close();

[Anikate-De]

The problem here is that even if the socket is closed, the server keeps listening to the request

request.listen((line)) {

and even when you encounter an empty line, it only adds the cookies list to the channel sink.

This seemed very peculiar to me, but I confirmed that it was indeed the problem by doing:
If you create a StreamSubscription and cancel it when an empty line is encountered.

late StreamSubscription reqSubscription;
reqSubscription = request.listen((line) {
  if (line.toLowerCase().startsWith('cookie:')) {
    cookies.add(line);
  }

  if (line.isEmpty) {
    // A blank line indicates the end of the headers.
    channel.sink.add(cookies);
    reqSubscription.cancel(); // <- here
  }
});

then all works well, without the need of explicitly removing idle connections.

[Anikate-De]

This is what I used for reference: https://stackoverflow.com/questions/31183730/okhttp-asynchronous-request-doesnt-stop-immediately-after-onresponse

[brianquinlan]

Hmmm... the test seems racy. Does this formulation work:

await for (final line in request) {
        if (line.toLowerCase().startsWith('cookie:')) {
          cookies.add(line);
        }

        if (line.isEmpty) {
          // A blank line indicates the end of the headers.
          channel.sink.add(cookies);
          break;
        }
}

socket.writeAll(...)
await socket.close()

[Anikate-De]

Yes, it does!

[Anikate-De]

Do you suggest changing the test case instead? I didn't go that route because CronetClient seems to pass both cases without a problem.

[brianquinlan]

Do you suggest changing the test case instead? I didn't go that route because CronetClient seems to pass both cases without a problem.

Yeah, I think that the test is incorrect and it is a coincidence that it works ;-)

[Anikate-De]

Alright, then it's good to close this PR. We would still need okhttp3.ConnectionPool and evictAll() for properly closing the client if I'm not wrong.

[brianquinlan]

Alright, then it's good to close this PR. We would still need okhttp3.ConnectionPool and evictAll() for properly closing the client if I'm not wrong.

You probably want to empty the connection pool when in response to Client.close(). From the docs

[Anikate-De]

Yeah, that's exactly what I was going to follow.

1. call shutdown on ExecutorService
2. empty the connection pool
3. release client object (JNI)