Adding handling for stream exhaustion in Netty. #585
Conversation
|
@ejona86 PTAL |
| @@ -184,14 +183,13 @@ public void cancelBufferedStreamShouldChangeClientStreamStatus() throws Exceptio | |||
|
|
|||
| assertTrue(createPromise.isSuccess()); | |||
| verify(stream).transportReportStatus(eq(Status.CANCELLED), eq(true), | |||
| any(Metadata.Trailers.class)); | |||
| any(Metadata.Trailers.class)); | |||
There was a problem hiding this comment.
Are these whitespace changes necessary? It seems they are fine as-is.
There was a problem hiding this comment.
No .... it seems that the IDE's config for whitespace in this case is imperfect. It's probably more effort than it's worth to revert and there aren't may occurrences of this.
There was a problem hiding this comment.
The problem is that each IDE (even version) formats it differently, so you get constant churn. The constant useless churn that adds noise is what I have issue with. I'm thinking there should be a ban on full-file reformatting.
As a further example, the newline between the io.grpc and io.netty imports is minor but also improper, and so later we will have another PR that will fix it. The only way to stop the cycle it to stop committing useless code reformatting like here.
There was a problem hiding this comment.
I've reverted the formatting.
Agreed that full-file formatting is rather evil and should be avoided. I generally don't use it but must have in this case.
|
@ejona86 PTAL |
| + "Initiating graceful shutdown of the connection."); | ||
| try { | ||
| super.close(ctx, ctx.newPromise()); | ||
| } catch (Exception ce) { |
There was a problem hiding this comment.
Should we just add throws Exception to createStream() and write()? That seems more appropriate.
nmittler
force-pushed
the
streams_exhausted
branch
2 times, most recently
from
086f594
to
2f263ff
Compare
|
@ejona86 PTAL |
| promise.setFailure(e); | ||
| try { | ||
| super.close(ctx, ctx.newPromise()); | ||
| } catch (Exception ce) { |
There was a problem hiding this comment.
This isn't what I was encouraging when I was suggesting to add "throws Exception" to createStream() and write().
Instead, I was suggesting that this } catch (Exception ce) { doesn't need to be here at all, because write() can throws Exception. I do think the super.close() needs to be back where it was in createStream().
The changes for propagating StatusException further seem like a step backward. I liked it more with how it was before.
There was a problem hiding this comment.
Writes in Netty do not typically throw. Instead they fail the promise with the exception. We should be following that pattern.
I'll revert the code to the previous version.
|
@ejona86 PTAL ... I've reverted the exception handling logic in createStream. |
| try { | ||
| super.close(ctx, ctx.newPromise()); | ||
| } catch (Exception ce) { | ||
| // Promise was already failed. Just absorb. |
There was a problem hiding this comment.
In no way is the promise guaranteed to have failed.
There was a problem hiding this comment.
Not sure I follow ... we call promise.setFailure(e) above.
There was a problem hiding this comment.
Ah, I see. But this exception is being lost. We should really propagate it.
There was a problem hiding this comment.
If we ever move to java 7, this looks very similar to suppressed exceptions in the try-with clause. You might consider adding a todo to use e.addSuppressed(ce); if we ever expect to upgrade.
There was a problem hiding this comment.
Except that our method signature should simply "throws Exception". The only reason we have this exception is because Netty's close() has "throws Exception", and I see no reason why we shouldn't propagate.
|
@ejona86 PTAL |
|
@nmittler LGTM |
nmittler
force-pushed
the
streams_exhausted
branch
from
decb0a0
to
70b5b10
Compare
nmittler
force-pushed
the
streams_exhausted
branch
from
70b5b10
to
8fe5ac8
Compare
|
cherry-picked as 9f7cb80 |
No description provided.