Monomorphic clj-http tests for comparison

[KingMob]

I grabbed two of the more important test namespaces from clj-http, and altered the common request fn to simultaneously make requests with both clj-http and Aleph's client, and then compare the two, to the extent that it's possible.

This is a bit of a first draft, but there's already some interesting issues. I expect there might be some bugs in the test comparison themselves, so extra eyes could help.

For now, there's no attempt to compare the async mode of the clj-http client, but in theory we can test those too, with on-realized.

Notes:

1. One major difference is, when the response body is empty, clj-http returns nil, while Aleph returns an InputStream with no bytes. This was discussed in For 204 response the body should be empty... #321, but there's no further references to what happened, despite the issue being closed.
2. I'm seeing discrepancies in the numbers for multipart upload tests. I know one or both of you have looked into that more than I have. Multipart now works. Number discrepancies inherent in how multipart requests built.
3. There's some weird bug in the head-with-body test, where it works in clj-http's test suite, but not ours. I don't mean the Aleph call is failing, I mean the clj-http call is failing in our version of the test, for some reason. Dunno what that's about. I thought it was some weird Jetty caching issue, since it seems related to consuming an InputStream and then trying to reuse it a second time. This was due to the test being in an unreleased version. I shouldn't have copied straight from master...
4. There's some DNS resolver errors. Haven't even glanced at those. I took a look, and I'm not sure there's a good way to fix these, since they depend on Apache DNS classes, and not a map of settings. Luckily, I think custom DNS resolvers are pretty rare. Suggest we ignore.
5. There's a bunch of async tests in the client-test side that are failing or hanging, and they should work in the aleph env, but they're not. Probably a bug in my altered request fn in the util ns. Even if the tests are irrelevant to us (like the clj-http conn pool tests), there might be something there, since they're failing.
6. There's some NPE from t-request-without-url-set in ring-request->netty-request
7. The customMethodTest seems to hang.
8. In status-line-parsing, we don't return a :reason-phrase, which clj-http does. Done
9. Aleph doesn't support the :redirect-strategy option yet.
10. Aleph doesn't support :length yet. If specified, should not transmit more than :length bytes of the body.

Anyway, it's still a bit messy, but it would be nice to get some issues out of this, and fix some bugs. The nil body vs empty body inputstream also needs a bit of feedback. Do we go for clj-http parity, or do people rely on that difference?

[KingMob]

OK, middleware should be working. I had to alter how Aleph handles them to support it though. Aleph confusingly combined two different types of middleware. Since it's inherently async, most middleware alters a request map; however, wrap-request-timing and wrap-exceptions alter the actual client function used.

There's no observable behavior, but people relying on the default-middleware vector will now see it's been split into default-client-middleware and default-middleware. I hope this is OK, but I can revert if you think it's too much. E.g., I could leave default-middleware alone, but keep default-client-middleware around to support splitting default-middleware.

[KingMob]

I added a new :clj-http test selector, but unfortunately, it's hitting one of the tests that hang first thing.

Todo:

• Go through all the tests and add ^:ignore metadata to a lot of them.
• Override the client/* functions for comparing. Raw request is overriddent, but not things like client/get, client/post, etc.

[arnaudgeiser]

I'm seeing discrepancies in the numbers for multipart upload tests. I know one or both of you have looked into that more than I have.

Consider I will have a look at those tests next week.

[arnaudgeiser]

I'm seeing discrepancies in the numbers for multipart upload tests. I know one or both of you have looked into that more than I have.

Ok... this one is tricky.
There are some discrepancies between how Netty (aleph) generates the boundary and how
httpcomponents-client (clj-http) generates its one.
Both are correct regarding the RFC

The only mandatory parameter for the multipart Content-Type is the boundary parameter, which consists of 1 to 70 characters from a set of characters known to be very robust through email gateways, and NOT ending with white space. (If a boundary appears to end with white space, the white space must be presumed to have been added by a gateway, and should be deleted.) It is formally specified by the following BNF: [1]

Netty uses this to generates the boundary on HttpPostRequestEncoder [2]:

(Long/toHexString (.nextLong (io.netty.util.internal.PlatformDependent/threadLocalRandom)))
=> 624177513d77dcea

while httpcomponents-client generates this on MultipartEntityBuilder [3]:

(-> (org.apache.http.entity.mime.MultipartEntityBuilder/create)
    (.build)
    (.getContent)
    (bs/to-string))

=> "2FpTfsG9VvWrmeyeRtpEco02WjNrvZXqAzahT" ;; separators removed

My take would be to special case the multipart tests to not account the boundary
when doing the comparisons.

[1] : https://www.w3.org/Protocols/rfc1341/7_2_Multipart.html
[2] : https://github.com/netty/netty/blob/433f8ef8d50b4a5d793d5e879fc91561b23d8abb/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostRequestEncoder.java#L293
[3] : https://github.com/apache/httpcomponents-client/blob/master/httpclient5/src/main/java/org/apache/hc/client5/http/entity/mime/MultipartEntityBuilder.java#L193-L200

[KingMob]

@arnaudgeiser Thanks for digging into that.

Which do you think will be more robust?

1. Determining the boundary separators, stripping them out, and then comparing all remaining bytes?
2. Extracting the various parts, and then comparing each of them one-by-one to their counterparts?

1 seems simpler, but I haven't considered any further issues of compression or encoding. (OTOH, differences in either of those could lead to difference in bytes even without multipart boundaries, which I haven't seen, so maybe it's not an issue.)

[arnaudgeiser]

Yes, I would go straight for 1.
Hopefully it will be as easy as it seems.

[KingMob]

@arnaudgeiser Option 1 for multipart comparison won't work. Not only are the boundaries naturally different, since they're random strings, but the headers in each part differ. They differ by case, by order, and clj-http adds a Content-Length header that is uncommon, and has apparently caused problems elsewhere. We also name temp files, which get added as directives, differently. Even if we wanted to match the behavior, we couldn't do it for streaming bodies where we may not necessarily know the length when we write the headers.

It might be nice to parse the remaining headers, but Netty annoyingly buries that code in private inner classes a zillion hierarchies deep, so we'd have to use something else, and this is already getting a bit out of hand.

For now, I'm ignoring the headers and comparing just the multipart bodies. This turned up one bug, I think. In the tests multipart-inputstream-length and multipart-form-uploads, there's a byte array containing the string "byte-test". It's used in a few places, and doesn't seem to be transmitted up correctly afaict.

[arnaudgeiser]

I spent a bit of time on multipart-inputstream-length and if we are talking about the same thing, the bug is not on Aleph itself but how the test is shaped (not saying there is no mismatch there, just not the bug you are expecting).

(let [bytes (util/utf8-bytes "byte-test")
        stream (ByteArrayInputStream. bytes)
        ...]

Here we are constructing a ByteArrayInputStream which is consumes by both the aleph client and the clj-http one.
As we are doing both requests with the same stream, only one client can use the data.

In the current situation, the clj-http client got the data.

But if you shuffle those two lines [2] aleph will get the data while clj-http will hang waiting
for data and will end eventually on a timeout.

clj-http-resp (clj-http-request clj-http-ring-map)
aleph-resp @(http/request aleph-ring-map)

[1] : https://github.com/clj-commons/aleph/pull/628/files#diff-5b192cc1db92f511c86757b4e7cabb888312e80a95ed9156d053054a210a1172R354
[2] : https://github.com/clj-commons/aleph/pull/628/files#diff-1d4fff9aa4a55859c8984ea9b1ad9eb1806ebeb9d6e5850a70f3bd94d1d1a915R254-R255

[arnaudgeiser]

@arnaudgeiser Option 1 for multipart comparison won't work. Not only are the boundaries naturally different, since they're random strings, but the headers in each part differ. They differ by case, by order, and clj-http adds a Content-Length header that is uncommon, and has square/okhttp#2604 square/okhttp#2138. We also name temp files, which get added as directives, differently. Even if we wanted to match the behavior, we couldn't do it for streaming bodies where we may not necessarily know the length when we write the headers.

Let's not burn too much time on this issue.
Whether we are able to compare the returned content of the parts, the mime-type (where the are some mismatch somehow last time I look at it), the length, the encoding and the name, I will consider the implementation to be good enough.

[KingMob]

As we are doing both requests with the same stream, only one client can use the data.

Ahh, good catch. Ironically, I ran into that exact issue earlier on, with needing to read clj-http's response stream twice, which is why I made tee-output-stream. But since the multipart stream is a ByteArrayInputStream, we can easily make a copy and call .reset() on the original.

Just pushed a fix.

[arnaudgeiser]

Do you want me to take over this Pull Request?
Since I'm working on the error-handling branch, I'm happy to help to reach the finish line.

[KingMob]

Thanks, but I've got it. For the most part, it's turned up little new errors (we already knew we were pretty compatible with clj-http), so there's no urgency.

[KingMob]

Unless... you're dying to work on it?

[arnaudgeiser]

Unless... you're dying to work on it?

Not at all.
I would like to clean all the issues and close all the PRs before diving into HTTP 2.0.