fix #4201: generalizing sendAsync support #4430
Conversation
|
Here's where things stand - sendAsync is now implemented by SendAsyncUtils and several helper classes designed to run over top of consumeBytes - HttpClientReadableByteChannel (ReadableByteChannel / InputStream / Reader) and ByteArrayBodyHandler (byte[] and String). The consumeLines and asyncBody tests were refined to span multiple result consumptions - this highlighted that the JDK implementation needs to immediately deliver the AsyncBody result for the InputStream / Reader handling, which was the general expectation for sendAsync calls returing InputStream/Reader. Jetty support was refined to remove blocking logic - we will use flow consumption, just like with the JDK implementation. The JDK. |
…alls also converting jetty to fully non-blocking and adapting the body delivery for jdk for use with sendAsync logic - we need the AsyncBody to be available immediately
|
@manusa I think this pr addresses some of the test inconsistency seen before. The jdk logic had an issue with consumeBytes not immediately returning the AsyncBody that is addressed by https://github.com/fabric8io/kubernetes-client/pull/4430/files#diff-291f7471cf12ec2bdc6787de6331b77befeca706eaf6ebfe6fdcd5110f3a3560R67 - which I think may have accounted for some of the timing issues with not seeing log results. |
adding missing test timeouts correcting the wait in HttpClientReadableByteChannel
| @@ -115,6 +115,7 @@ public void onWebSocketClose(int statusCode, String reason) { | |||
|
|
|||
| @Override | |||
| public void onWebSocketConnect(Session session) { | |||
| this.webSocketSession = session; | |||
There was a problem hiding this comment.
Could you please elaborate on this change?
I have a fade memory having trouble doing sth similar (but can't remember exactly right now, maybe accessing the session before it was set)
There was a problem hiding this comment.
When I started looking at my failed jetty test, I reviewed the implementation more. This change mirrors the logic Jetty's WebSocketAdapter - in particular making the session volatile.
There was a problem hiding this comment.
I cannot recall what the problem was at the time, but seems to be working fine with the tests 👍
There was a problem hiding this comment.
Also onWebSocketConnect is called before the websocketclient.connect future is completed.
There was a problem hiding this comment.
Yes, as I said I can't remember exactly what or why I placed that there.
Maybe it was just an issue with the future chaining method, dunno, anyway doesn't matter.
| .extracting(HttpResponse::code).isEqualTo(400); | ||
| assertThat(result) |
There was a problem hiding this comment.
did you change this because it failed in Java17? or is there any other reason?
There was a problem hiding this comment.
a single assertion can be preserved using .returns(400, HttpResponse::code)
There was a problem hiding this comment.
This was needed due to the response wrapping - with some additional refactoring that could be avoided.
There was a problem hiding this comment.
OK, then I'll replace with the returns DSL method that should work
- implementation of JettyHttpClient consumeLines - additional tests - centralization of HttpResponse body/async consumption logic - uncalled-for nitpicks 😅😇 Signed-off-by: Marc Nuri <[email protected]>
review: HttpClient implementation refactor
|
SonarCloud Quality Gate failed. |
Description
Fix #4201
Fix #4564
This is to address #4201. It keeps the calling contract the same and introduces default logic built on top of the consumeBytes method. We need two new handlers for that - one for collecting all of the bytes, and one to act as a ReadableByteChannel (which is convertable to input stream). We may later want to refine this so that the HttpResponseAdapter isn't necessary by separating the responsibility of the body representation from the HttpResponse.
Most of the logic related to sendAsync in the client implementations was removed.
To cut down on the variation in the implementations, we'll forcibly use the UTF-8 charset (jetty was looking at the encoding, which doesn't typically specify the charset, while okhttp and jdk are parsing the content-type).
We also have an issue with the jetty consumeLines implementation - it isn't actually splitting the lines. Since this is only used for http watches, I just converted it to unsupported for now.
This will also help smooth over the vert.x client support a little bit - it will no longer need to directly support InputStream/Reader.
Type of change
test, version modification, documentation, etc.)
Checklist