Apache HTTP async client metrics use HttpContext in weird way, NPE is thrown

[flozano]

Describe the bug
I am getting a NPE when using properly (I think, as I follow the javadoc in that class) MicrometerHttpClientInterceptor.

See #1886 (comment) for all the details; I was initially thinking it was just a weird design decision and therefore just commented there, but now I am convinced it's a bug (after reviewing the different implementations of HttpContext interface in apache httpcomponents client 4.x, I am not sure it can be safely used as a map key).

Environment
JDK 17
MacOS
Apache httpcomponents async client 4.x

To Reproduce
use the interceptor exactly as described.

Additional context
all the details are in #1886 (comment)

[shakuzen]

I don't know a particular reason that implementation was chosen. I also don't know why you would be getting a NPE because of it, though. Is this something you can reproduce in a test case? I think the context would need to be shared by multiple requests for that to happen. And if that is the case, I wonder why we wouldn't have the same issue if we try storing the Micrometer sample in the HttpContext. Though I agree it does seem like a reasonable approach, beside this issue.

[flozano]

I was not successful reproducing it in a test, but it shows up consistently in a deployed server.

First thing I need to look at is that this client sends a BasicHttpContext containing some authentication configuration. Maybe the HttpContext seen by the initial interceptor is the one I send, and the response interceptor for some reason sends an IOSession-backed one.

Another wild speculation is that for some reason (number of I/O threads, time of server to respond), the server takes a while to respond and the HttpContext is not exactly the same instance (it's backed by same IOSession and therefore contains the same attributes, but it's not the same HttpContext instance) because the client suspends the request. This is just guess, as it's the only explanation I can think of.

Or maybe it's a combination of both. The NPE however definitely happens, and the interceptors are properly set to the client, so something is going on.

If any of those (or both) was the cause, by using the interface of the HttpContext it should be fixed, as it is a stronger expectation on the API contract that the HttpContext interface provides the same bag of attributes to both the request and the response interceptor. Current code has an expectation about the HttpContext not only containing the same info, but also about being exactly the same instance when received by both interceptors. I am not sure it can be assumed this is the case.

I will keep trying to reproduce in isolated way it by tweaking httpcomponents async client I/O reactor and server response time.

[cachescrubber]

In #3801 instrumentation is using an AyncExecChainHandler and does not depend on the HttpContext.

[bclozel]

@flozano I'm afraid I haven't found a way to properly fix this in the 4.x generation of the client. We cannot apply the same fix as #3801 as the contracts don't exist. See #3932 for context: we might drop support for the 4.x async client as a result.

[flozano]

I understand. Thanks for the insight. I think it is fine if only 4.x async support is dropped. For my projects it's not a problem, as I'm moving everything to 5.x. My main usage of 4.x async was Spring's AsyncRestTemplate - which is gone :)

[bclozel]

Superseded by #3932 for HttpComponents 4.x and #3800 for HttpComponents 5.x.