Handle streaming exceptions as bad requests instead of internal errors #178
Conversation
thinaih
force-pushed
the
handle-stream-errors-as-client-errors
branch
from
0eb1106 to
d7b16a8
Compare
| @@ -61,7 +60,7 @@ public int read() | |||
|
|
|||
| int i = delegate.read(); | |||
| if (i < 0) { | |||
| throw new EOFException("Unexpected end of stream"); | |||
| throw new AwsProxyStreamException("Unexpected end of stream"); | |||
There was a problem hiding this comment.
Is there an existing AWS SDK Exception we can throw?
There was a problem hiding this comment.
There is the SdkServiceException or its subclass AwsServiceException. However I didn't want to use them since I think it might be better to separate the exceptions thrown by the proxy stream classes and handle them differently. Creating this new exception makes it possible to do so without the risk of swallowing other exceptions that might need to be handled differently. wdyt?
There was a problem hiding this comment.
Then maybe WebApplicationException is better. I'm generally against defining bespoke exceptions unless absolutely necessary.
There was a problem hiding this comment.
I think that this class doesn't need to know that this should be handled as a 400 or something else, there is also an issue #73 that aims to separate internal exceptions from the http logic, and throwing a WebApplicationException here, will not take us closer to addressing that issue.
There was a problem hiding this comment.
That's my opinion. I don't think we should be defining new exceptions.
There was a problem hiding this comment.
I agree with Jordan here, creating custom exception classes that don't have extra fields or functions makes the code more convoluted.
This class is part of the proxy's "private" core and is pretty close to networking code, so throwing a WebApplicationException makes sense
Currently the proxy handles most streaming issues due to mismatch between declared content length and uploaded content length as INTERNAL_ERRORs, except for when it's a STANDARD or W3C_CHUNKED and uploaded > content length, then this is handled as UNAUTHORIZED.
This PR is suggesting handle all of the above as BAD_REQUEST.