-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Progress reporter promoted to core and used in Http Client. #29495
Changes from 15 commits
a6c25be
ce1a37c
528a324
38a4292
cb2275b
7f40ad0
e828a3b
2e84598
1940661
3f585e3
36f4d3c
a323065
559444b
b2308ea
6eae4f5
054a1b8
5f779a8
551bef7
7affa8f
c0b305f
f5b0647
64e33ac
3c5e054
c9ec937
a37ef28
bc331bc
bfdd5b1
3bc97cd
67837d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ | |
import com.azure.core.http.netty.implementation.NettyAsyncHttpResponse; | ||
import com.azure.core.http.netty.implementation.NettyToAzureCoreHttpHeadersWrapper; | ||
import com.azure.core.http.netty.implementation.ReadTimeoutHandler; | ||
import com.azure.core.http.netty.implementation.RequestProgressReportingHandler; | ||
import com.azure.core.http.netty.implementation.ResponseTimeoutHandler; | ||
import com.azure.core.http.netty.implementation.WriteTimeoutHandler; | ||
import com.azure.core.implementation.util.BinaryDataContent; | ||
|
@@ -23,7 +24,9 @@ | |
import com.azure.core.implementation.util.StringContent; | ||
import com.azure.core.util.BinaryData; | ||
import com.azure.core.util.Context; | ||
import com.azure.core.util.Contexts; | ||
import com.azure.core.util.FluxUtil; | ||
import com.azure.core.util.ProgressReporter; | ||
import com.azure.core.util.logging.ClientLogger; | ||
import io.netty.buffer.ByteBuf; | ||
import io.netty.buffer.Unpooled; | ||
|
@@ -114,8 +117,8 @@ public Mono<HttpResponse> send(HttpRequest request, Context context) { | |
.orElse(this.responseTimeout); | ||
|
||
return nettyClient | ||
.doOnRequest((r, connection) -> addWriteTimeoutHandler(connection, writeTimeout)) | ||
.doAfterRequest((r, connection) -> addResponseTimeoutHandler(connection, effectiveResponseTimeout)) | ||
.doOnRequest((r, connection) -> addRequestHandlers(connection, context)) | ||
.doAfterRequest((r, connection) -> doAfterRequest(connection, effectiveResponseTimeout)) | ||
.doOnResponse((response, connection) -> addReadTimeoutHandler(connection, readTimeout)) | ||
.doAfterResponseSuccess((response, connection) -> removeReadTimeoutHandler(connection)) | ||
.request(HttpMethod.valueOf(request.getHttpMethod().toString())) | ||
|
@@ -282,19 +285,30 @@ private static BiFunction<HttpClientResponse, Connection, Publisher<HttpResponse | |
} | ||
|
||
/* | ||
* Adds write timeout handler once the request is ready to begin sending. | ||
* Adds request handlers: | ||
* - write timeout handler once the request is ready to begin sending. | ||
* - progress handler if progress tracking has been requested. | ||
*/ | ||
private static void addWriteTimeoutHandler(Connection connection, long timeoutMillis) { | ||
connection.addHandlerLast(WriteTimeoutHandler.HANDLER_NAME, new WriteTimeoutHandler(timeoutMillis)); | ||
private void addRequestHandlers(Connection connection, Context context) { | ||
connection.addHandlerLast(WriteTimeoutHandler.HANDLER_NAME, new WriteTimeoutHandler(writeTimeout)); | ||
ProgressReporter progressReporter = Contexts.with(context).getProgressReporter(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we clarify what determines if something that is user-settable via a Context becomes elevated to be an API on Contexts? For example, I see above code that gets the AZURE_RESPONSE_TIMEOUT and AZURE_EAGERLY_READ_RESPONSE values out of Context. At what stage do we do this elevation, and then, does progress reporter meet that criteria? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd say that stuff that has to cross core-sdks boundary is a candidate where HLC logic has interest in orchestrating it.
|
||
connection.removeHandler(RequestProgressReportingHandler.HANDLER_NAME); | ||
alzimmermsft marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (progressReporter != null) { | ||
connection.addHandlerLast( | ||
alzimmermsft marked this conversation as resolved.
Show resolved
Hide resolved
|
||
RequestProgressReportingHandler.HANDLER_NAME, new RequestProgressReportingHandler(progressReporter)); | ||
} | ||
} | ||
|
||
/* | ||
* Remove write timeout handler from the connection as the request has finished sending, then add response timeout | ||
* After request has been sent: | ||
* - Remove Progress Handler | ||
* - Remove write timeout handler from the connection as the request has finished sending, then add response timeout | ||
* handler. | ||
*/ | ||
private static void addResponseTimeoutHandler(Connection connection, long timeoutMillis) { | ||
private static void doAfterRequest(Connection connection, long responseTimeoutMillis) { | ||
connection.removeHandler(RequestProgressReportingHandler.HANDLER_NAME); | ||
connection.removeHandler(WriteTimeoutHandler.HANDLER_NAME) | ||
.addHandlerLast(ResponseTimeoutHandler.HANDLER_NAME, new ResponseTimeoutHandler(timeoutMillis)); | ||
.addHandlerLast(ResponseTimeoutHandler.HANDLER_NAME, new ResponseTimeoutHandler(responseTimeoutMillis)); | ||
} | ||
|
||
/* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT License. | ||
|
||
package com.azure.core.http.netty.implementation; | ||
|
||
import com.azure.core.util.ProgressReporter; | ||
import io.netty.buffer.ByteBuf; | ||
import io.netty.buffer.ByteBufHolder; | ||
import io.netty.channel.ChannelHandlerContext; | ||
import io.netty.channel.ChannelOutboundHandlerAdapter; | ||
import io.netty.channel.ChannelPromise; | ||
import io.netty.channel.FileRegion; | ||
|
||
import java.util.Objects; | ||
|
||
public final class RequestProgressReportingHandler extends ChannelOutboundHandlerAdapter { | ||
/** | ||
* Name of the handler when it is added into a ChannelPipeline. | ||
*/ | ||
public static final String HANDLER_NAME = "azureRequestProgressHandler"; | ||
|
||
private final ProgressReporter progressReporter; | ||
|
||
public RequestProgressReportingHandler(ProgressReporter progressReporter) { | ||
this.progressReporter = Objects.requireNonNull(progressReporter, "'progressReporter' must not be null"); | ||
} | ||
|
||
@Override | ||
public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise promise) { | ||
|
||
if (msg instanceof ByteBuf) { | ||
progressReporter.reportProgress(((ByteBuf) msg).readableBytes()); | ||
} else if (msg instanceof ByteBufHolder) { | ||
progressReporter.reportProgress(((ByteBufHolder) msg).content().readableBytes()); | ||
} else if (msg instanceof FileRegion) { | ||
progressReporter.reportProgress(((FileRegion) msg).count()); | ||
} | ||
|
||
ctx.write(msg, promise.unvoid()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT License. | ||
|
||
package com.azure.core.http.netty; | ||
|
||
import com.azure.core.http.HttpClient; | ||
import com.azure.core.test.HttpClientTestsWireMockServer; | ||
import com.azure.core.test.http.HttpClientTests; | ||
import com.github.tomakehurst.wiremock.WireMockServer; | ||
import io.netty.handler.ssl.SslContext; | ||
import io.netty.handler.ssl.SslContextBuilder; | ||
import io.netty.handler.ssl.util.InsecureTrustManagerFactory; | ||
import org.junit.jupiter.api.AfterAll; | ||
import org.junit.jupiter.api.BeforeAll; | ||
|
||
import javax.net.ssl.SSLException; | ||
|
||
/** | ||
* Reactor Netty {@link HttpClientTests} with https. | ||
* Some request logic branches out if it's https like file uploads. | ||
*/ | ||
public class NettyAsyncHttpClientHttpClientWithHttpsTests extends HttpClientTests { | ||
private static WireMockServer server; | ||
|
||
private static final HttpClient HTTP_CLIENT_INSTANCE; | ||
|
||
static { | ||
try { | ||
SslContext sslContext = SslContextBuilder.forClient() | ||
.trustManager(InsecureTrustManagerFactory.INSTANCE) | ||
.build(); | ||
|
||
reactor.netty.http.client.HttpClient nettyHttpClient = | ||
reactor.netty.http.client.HttpClient.create() | ||
.secure(sslContextSpec -> sslContextSpec.sslContext(sslContext)); | ||
|
||
HTTP_CLIENT_INSTANCE = new NettyAsyncHttpClientBuilder(nettyHttpClient).build(); | ||
} catch (SSLException e) { | ||
throw new RuntimeException(e); | ||
} | ||
} | ||
|
||
@BeforeAll | ||
public static void getWireMockServer() { | ||
server = HttpClientTestsWireMockServer.getHttpClientTestsServer(); | ||
server.start(); | ||
} | ||
|
||
@AfterAll | ||
public static void shutdownWireMockServer() { | ||
if (server != null) { | ||
server.shutdown(); | ||
} | ||
} | ||
|
||
@Override | ||
protected int getWireMockPort() { | ||
return server.httpsPort(); | ||
} | ||
|
||
@Override | ||
protected boolean isSecure() { | ||
return true; | ||
} | ||
|
||
@Override | ||
protected HttpClient createHttpClient() { | ||
return HTTP_CLIENT_INSTANCE; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,10 @@ | |
|
||
### Features Added | ||
|
||
- Added ability to track progress by passing `ProgressReporter` in the `Context`. | ||
I.e., `Contexts.with(context).setProgressReporter(progressReporter)` | ||
before calling `HttpClient.send(HttpRequest, Context)` API. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above |
||
|
||
### Breaking Changes | ||
|
||
### Bugs Fixed | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT License. | ||
|
||
package com.azure.core.http.okhttp.implementation; | ||
|
||
import com.azure.core.util.ProgressReporter; | ||
import okhttp3.MediaType; | ||
import okhttp3.RequestBody; | ||
import okio.Buffer; | ||
import okio.BufferedSink; | ||
import okio.ForwardingSink; | ||
import okio.Okio; | ||
import okio.Sink; | ||
|
||
import java.io.IOException; | ||
import java.util.Objects; | ||
|
||
/** | ||
* An {@link okhttp3.RequestBody} subtype that adds progress | ||
* reporting functionality on top of other {@link okhttp3.RequestBody}. | ||
*/ | ||
public class OkHttpProgressReportingRequestBody extends RequestBody { | ||
private final RequestBody delegate; | ||
private final ProgressReporter progressReporter; | ||
|
||
public OkHttpProgressReportingRequestBody(RequestBody delegate, ProgressReporter progressReporter) { | ||
this.delegate = Objects.requireNonNull(delegate, "'delegate' must not be null"); | ||
this.progressReporter = Objects.requireNonNull(progressReporter, "'progressReporter' must not be null"); | ||
} | ||
|
||
@Override | ||
public MediaType contentType() { | ||
return delegate.contentType(); | ||
} | ||
|
||
@Override | ||
public long contentLength() throws IOException { | ||
return delegate.contentLength(); | ||
} | ||
|
||
@Override | ||
public boolean isOneShot() { | ||
return delegate.isOneShot(); | ||
} | ||
|
||
@Override | ||
public boolean isDuplex() { | ||
return delegate.isDuplex(); | ||
} | ||
|
||
@Override | ||
public void writeTo(BufferedSink sink) throws IOException { | ||
BufferedSink bufferedSink; | ||
|
||
CountingSink countingSink = new CountingSink(sink, progressReporter); | ||
bufferedSink = Okio.buffer(countingSink); | ||
|
||
delegate.writeTo(bufferedSink); | ||
|
||
bufferedSink.flush(); | ||
} | ||
|
||
private static final class CountingSink extends ForwardingSink { | ||
private final ProgressReporter progressReporter; | ||
|
||
CountingSink(Sink delegate, ProgressReporter progressReporter) { | ||
super(delegate); | ||
this.progressReporter = progressReporter; | ||
} | ||
|
||
@Override | ||
public void write(Buffer source, long byteCount) throws IOException { | ||
super.write(source, byteCount); | ||
progressReporter.reportProgress(byteCount); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things:
getContext()
and pass that in, rather than expect to mutate the existingContext
passed in to thewith
method with the additional progressReporter. I sat here for a second thinking "do we have two different usage patterns here?" before concluding that the naive (and incorrect, but only with knowledge about how Context works) interpretation of the code sample above would be to do the following:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to sample, see effect here https://github.com/kasobol-msft/azure-sdk-for-java/blob/progress-reporter/sdk/core/azure-core-http-netty/CHANGELOG.md#features-added