Adding AWS-CRT-Java 0.4.20 to AWS-SDK-Java-v2 #1607
Conversation
Factoring out EventLoopGroup/HostResolver from client constructor, fixing bug reported by Sonar
rccarper
force-pushed
the
aws-crt-latest-api-update
branch
from
285e671 to
f7a9464
Compare
rccarper
changed the title
Codecov Report
@@ Coverage Diff @@
## master #1607 +/- ##
============================================
- Coverage 76.34% 75.91% -0.44%
- Complexity 182 265 +83
============================================
Files 1070 1074 +4
Lines 32225 32301 +76
Branches 2520 2500 -20
============================================
- Hits 24601 24520 -81
- Misses 6388 6528 +140
- Partials 1236 1253 +17
Continue to review full report at Codecov.
|
rccarper
force-pushed
the
aws-crt-latest-api-update
branch
2 times, most recently
from
4a72926 to
d554587
Compare
rccarper
force-pushed
the
aws-crt-latest-api-update
branch
from
d554587 to
d421878
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
rccarper
changed the title
|
I needed to make a small one line fix to get this to work with aws-crt-java v0.5.1: alexw91@025c43c Feel free to incorporate that fix into this PR. |
…s. Also fixed recognition of parameterized classes as potential document beans
… basic operations
| * @param windowSize The AWS Common Runtime WindowSize | ||
| * @return The builder of the method chaining. | ||
| */ | ||
| Builder windowSize(int windowSize); |
There was a problem hiding this comment.
I think we should rename this as it may confuse users when HTTP/2 support is added, which also has its own concept of a window size.
There was a problem hiding this comment.
crtWindowSize enough or do we need something completely different?
There was a problem hiding this comment.
I think having windowSize at all in there would be confusing. As far as I understand, this is how much data the CRT client buffers for reads right? Maybe something like "pendingReadBuffer"? Or something along those lines.
There was a problem hiding this comment.
On further investigation, this parameter will also control h/2 initial window size so it should remain the same. I'm going to change it to initialWindowSize though to more closely match its function.
| * @param verifyPeer true if the Certificate Chain should be validated, false if validation should be skipped. | ||
| * @return The builder of the method chaining. | ||
| */ | ||
| Builder verifyPeer(boolean verifyPeer); |
There was a problem hiding this comment.
We should rely on the standard TRUST_ALL_CERTIFICATES options here
| /** | ||
| * Builder that allows configuration of the AWS CRT HTTP implementation. | ||
| */ | ||
| public interface Builder extends SdkAsyncHttpClient.Builder<AwsCrtAsyncHttpClient.Builder> { |
There was a problem hiding this comment.
Will the client have support for various timeout configurations? e.g. connection and read timeouts?
Proxy support is also an important one for customers
There was a problem hiding this comment.
We have connection (socket) timeouts and throughput monitoring which technically is capable of expressing a read timeout is a specialization. Neither appear to be exposed here though.
Basic proxy support exists, but no socks yet. I'll look into exposing proxy configuration.
There was a problem hiding this comment.
Coo proxy support would be nice
| private final int maxConnectionsPerEndpoint; | ||
|
|
||
|
|
||
| public AwsCrtAsyncHttpClient(DefaultBuilder builder, AttributeMap config) { |
| * <p>This can be created via {@link #builder()}</p> | ||
| */ | ||
| @SdkPublicApi | ||
| public class AwsCrtAsyncHttpClient implements SdkAsyncHttpClient { |
|
|
||
| private List<HttpHeader> createHttpHeaderList(URI uri, AsyncExecuteRequest asyncRequest) { | ||
| SdkHttpRequest sdkRequest = asyncRequest.request(); | ||
| List<HttpHeader> crtHeaderList = new ArrayList<>(sdkRequest.headers().size() + 2); |
There was a problem hiding this comment.
minor: what's the reason for the + 2?
There was a problem hiding this comment.
Looks like it's just a reserve on the potential extra headers added, but then again, there's 3 potential additions, so a little weird. Could change to 3 and add a note.
|
|
||
| if (!wasFirstSubscriber) { | ||
| log.error(() -> "Only one subscriber allowed"); | ||
| subscriber.onError(new IllegalStateException("Only one subscriber allowed")); |
There was a problem hiding this comment.
onSubscribe must be called before signalling onError. See 1.9: https://github.com/reactive-streams/reactive-streams-jvm/blob/v1.0.3/README.md
There was a problem hiding this comment.
Unsure of the right way to proceed here. We only want one subscriber so should we just drop the onError signal and return, or drop it and throw the exception that was being pushed? The latter seems best, but I want to check first.
There was a problem hiding this comment.
You can still signal onError for all subscribers besides the first one. The spec just requires that onSubscribe is called before signalling anything. The typical way to handle this would be to simply call onSubscribe with a dummy Subscription:
if (!wasFirstSubscriber) {
subscriber.onSubscribe(new DummySubscription());
subscriber.onError(...);
}This mentioned in the doc:
...for all other situations the only legal way to signal failure (or reject the Subscriber) is by calling onError (after calling onSubscribe).
| * @param <T> The CrtResource Type | ||
| * @return The CrtResource passed in | ||
| */ | ||
| private <T extends CrtResource> T own(T subresource) { |
There was a problem hiding this comment.
Is the type param needed? Can it just accept and return CrtResource?
There was a problem hiding this comment.
We do use the function as the right-hand value of assignment so technically it's used.
|
|
||
| HttpHeader[] crtHeaderArray = asArray(createHttpHeaderList(uri, asyncRequest)); | ||
|
|
||
| return new HttpRequest(method, encodedPath + encodedQueryString, crtHeaderArray, crtToSdkAdapter); |
There was a problem hiding this comment.
Encoded path may be null or empty here, I think we need to account for that here and defeault to / if so
| if (connPool == null) { | ||
| HttpClientConnectionManager newConnPool = createConnectionPool(uri); | ||
| HttpClientConnectionManager alreadyExistingConnPool = connectionPools.putIfAbsent(uri, newConnPool); | ||
|
|
||
| if (alreadyExistingConnPool == null) { | ||
| connPool = newConnPool; | ||
| } else { | ||
| // Multiple threads trying to open connections to the same URI at once, close the newer one | ||
| newConnPool.close(); | ||
| connPool = alreadyExistingConnPool; | ||
| } | ||
| } |
There was a problem hiding this comment.
Is it relatively expensive to create/close a connection pool? Given that this is not likely to be a highly contented call, it might be better to synchronize on create
There was a problem hiding this comment.
It is not connecting but it is creating a tls context. What kind of synchronization would you think appropriate? A single lock for the client when creating new pools?
There was a problem hiding this comment.
I think just making the whole method synchronized would be fine. WDYT?
There was a problem hiding this comment.
Went with a slightly different approach: synchronized is used to make close() and getOrCreateConnectionPool() mutually exclusive, but we also add-ref on the returned pool and let a try-with-resources block perform the corresponding dec-ref on the pool after the request was submitted. Essentially in the time between when the pool was fetched and when the request was submitted, someone could call close and shutdown the pool from underneath us. This prevents that (in the sense that the pool won't shut down until the connection acquisition has been submitted).
|
SonarCloud Quality Gate failed.
|
Codecov Report
@@ Coverage Diff @@
## master #1607 +/- ##
============================================
+ Coverage 75.83% 75.91% +0.07%
- Complexity 182 265 +83
============================================
Files 1069 1074 +5
Lines 32033 32301 +268
Branches 2490 2500 +10
============================================
+ Hits 24291 24520 +229
- Misses 6510 6528 +18
- Partials 1232 1253 +21
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
|
Replaced by #1777. |
…3b8c5e991 Pull request: release <- staging/f0def870-d7b7-4175-9377-2d03b8c5e991
Description
This was done by rebasing master onto crt-api-update, then rebasing crt-api-update onto aws-crt-dev-preview, making some additional changes to account for the latest CRT API, and then creating this branch. I'm open to merging this into aws-crt-dev-preview first if that's preferable to merging everything into master.
As such, there are a lot of changes here not done by myself. Only the changes done in commit f7a9464 are, in that sense, actually new. More detail about those changes:
Testing
mvn clean install
[x ] I confirm that this pull request can be released under the Apache 2 license