Improved Expect: 100-continue response handling in the URL connection client. #3050
Conversation
millems
force-pushed
the
millem/improve-expect-100-continue
branch
from
fd70636 to
2b4fba5
Compare
| private boolean exceptionCausedBy100HandlingBug(RuntimeException e) { | ||
| return requestWasExpect100Continue() && | ||
| e.getMessage() != null && | ||
| e.getMessage().contains("Server rejected operation"); |
There was a problem hiding this comment.
Nit: Checking for ProtocolException first may prevent some unnecessary string comparisons here.
| EmbeddedServer server = new EmbeddedServer(handler)) { | ||
| HttpExecuteResponse response = sendRequest(client, server); | ||
| assertThat(response.httpResponse().statusCode()).isEqualTo(500); | ||
| assertThat(response.httpResponse().firstMatchingHeader("x-amz-test-header")).hasValue("foo"); |
There was a problem hiding this comment.
Nit: Could also assert a warning was logged with LogCaptor.
| return getAndHandle100Bug(() -> invokeSafely(connection::getErrorStream), Expect100BugHandlingBehavior.LOG); | ||
| } | ||
|
|
||
| private <T> Optional<T> getAndHandle100Bug(Supplier<T> supplier, Expect100BugHandlingBehavior bugBehavior) { |
There was a problem hiding this comment.
Nice helper method! A small description of the bug may be helpful for posterity's sake.
millems
force-pushed
the
millem/improve-expect-100-continue
branch
from
8348b07 to
514ae13
Compare
… client. HTTPURLConnection currently throws a ProtocolException if the service returns anything other than a 100 or 417 response code to an expect-100-continue request. S3 sometimes triggers this behavior. This change catches this exception and treats it according to its response code, assuming that it can verify that no information was lost (e.g. because of a response payload being included in the response). If it still fails, it instructs the customer that the apache HTTP client could be a true solution to the problem.
millems
force-pushed
the
millem/improve-expect-100-continue
branch
from
514ae13 to
adac996
Compare
| throw e; | ||
| } | ||
|
|
||
| expect100BugEncountered = true; |
There was a problem hiding this comment.
Should we put responseHasNoContent() before this, so that we only emit warnings when we're actually missing some potential behavior?
There was a problem hiding this comment.
Sure. responseHasNoContent() is relatively slow to check, so I'll have to rearrange some things to ensure we don't check it twice in the success case.
| private Optional<InputStream> tryGetErrorStream() { | ||
| InputStream result = invokeSafely(connection::getErrorStream); | ||
| if (result == null && expect100BugEncountered) { | ||
| log.debug(() -> "The response payload has been dropped because of a limitation of the JDK's URL Connection " |
There was a problem hiding this comment.
Is there anyway we can trace/correlate this to a given request? Should we include the status line?
There was a problem hiding this comment.
Since it's a sync client, they can use the thread name?
| } | ||
|
|
||
| @Test | ||
| public void expect100ContinueWorksWithZeroContentLength500() throws Exception { |
There was a problem hiding this comment.
Nit: Should we assert that no warnings were logged?
There was a problem hiding this comment.
Is that to prevent someone upgrading the log to warn?
There was a problem hiding this comment.
I meant to assert that nothing was logged at all (if there was no missing behavior).
|
Kudos, SonarCloud Quality Gate passed! |
…f6f8acfeb Pull request: release <- staging/a801662b-81c4-4e38-a626-c01f6f8acfeb
HTTPURLConnection currently throws a ProtocolException if the service returns anything other than a 100 or 417 response code to an expect-100-continue request. S3 sometimes triggers this behavior. This change catches this exception and treats it according to its response code, assuming that it can verify that no information was lost (e.g. because of a response payload being included in the response). If it still fails, it instructs the customer that the apache HTTP client could be a true solution to the problem.