Conversation
* Add CRT HTTP/2 support * Update CRT version and re-add back comments # Conflicts: # pom.xml
| private final HttpExecuteRequest request; | ||
| private final long readBufferSize; | ||
| private final HttpClientConnectionManager crtConnPool; | ||
| private final HttpStreamManager crtConnPool; |
There was a problem hiding this comment.
trivial: maybe rename this? It's not a connection pool anymore?
| // TODO: check with CRT team, could CRT_SOCKET_TIMEOUT be thrown | ||
| // from processes other than tcp connect? |
There was a problem hiding this comment.
i am not sure I follow this questions?
https://github.com/awslabs/aws-crt-java/pull/928/changes#diff-48bd13e1ade21ddbc9fb312b7f0dfdafb183e1fd59058919ca2c55ce53987ce2R504
We recently added this method to better identify the error is supported to be retry or not.
We basically take a list from io, here, and a list from http, here, to be retryable and everything else is not retryable. (Note that we took AWS_IO_SOCKET_TIMEOUT as retryable)
I think this logic should probably be something like:
- if the error code is retryable, wrap it with retry
- Else if error is CRT_TLS_NEGOTIATION_ERROR_CODE, wrap it with ssl exception
- Anything else is probably a connect exception
There was a problem hiding this comment.
Sure, I'll update the code to use CRT.awsIsTransientError()) instead. The reason we have this logic is to special case (retryable) errors from TCP connect, my question is: is there a way to tell if a retryable error is from TCP connect? If we go with the logic you proposed, it'd be wrapped with a generic IOException in step 1
| ClientBootstrap clientBootstrap = new ClientBootstrap(null, null); | ||
| SocketOptions clientSocketOptions = buildSocketOptions(builder.getTcpKeepAliveConfiguration(), | ||
| config.get(SdkHttpConfigurationOption.CONNECTION_TIMEOUT)); | ||
| TlsContextOptions clientTlsContextOptions = | ||
| TlsContextOptions.createDefaultClient() | ||
| .withCipherPreference(resolveCipherPreference(builder.getPostQuantumTlsEnabled())) | ||
| .withVerifyPeer(!config.get(SdkHttpConfigurationOption.TRUST_ALL_CERTIFICATES)); |
There was a problem hiding this comment.
why we remove the try with resource block? Maybe I am not following the logic, but where do we clean up these resource?
There was a problem hiding this comment.
So previously, we have this subresource.addRef() process to increase reference every time we use it and then try with resource to close it. I just removed both to keep the logic simple.
2 participants
Merge CRT H2 Support