Add throwTranslatedWriteException, refactoring, async helper #1379
Conversation
katcharov
force-pushed
the
JAVA-5402-refactor
branch
from
d13b0f2
to
0f9299e
Compare
| if (isClosed()) { | ||
| throw new MongoSocketClosedException("Cannot write to a closed stream", getServerAddress()); | ||
| } | ||
| c.complete(c); |
There was a problem hiding this comment.
The current implementation of the complete method requires the callback object to be passed to itself c.complete(c);. As i see, this pattern is intended to prevent accidental misuse of complete(T) when a result is not meant to be returned, as the method asserts that the callback is the same as the instance (Assertions.assertTrue(callback == this);). However, this pattern can be confusing and counterintuitive.
Wouldn't it be more straightforward to simplify this method to a parameterless complete()? This would make the method's usage intuitive c.complete();, eliminating the awkwardness of having to pass the callback to itself.
There was a problem hiding this comment.
The purpose is actually the opposite, to prevent misuse of the void complete() when a result should be returned. This is a workaround to force a typecheck on the return type. It is very easy to make mistakes in the boilerplate (calling the wrong callback, passing the wrong value, and so on), whether in the old or new async code. I think it is important to prevent this (which is a correctness issue, and constant problem), even if it makes the API unusual (which is only ugly but not a correctness issue, and we only have to pay the price of getting used to this a few times).
| } | ||
| } | ||
|
|
||
| private void affected(final int i) { |
There was a problem hiding this comment.
The affected and affectedReturns methods currently contain duplicate code. To simplify this, the body of the affected method can be refactored as follows:
private void affected(final int i) {
affectedReturns(i);
}
Th same applies to plain and plainReturns.
There was a problem hiding this comment.
I have made them not have the same code.
| }, Exception.class, (e, c) -> { | ||
| close(); | ||
| throwTranslatedWriteException(e); | ||
| }).finish(callback); |
There was a problem hiding this comment.
Once an error is thrown from this location throwTranslatedWriteException(e), it gets pushed to the callback. If the callback itself throws an error, it will then be propagated out of this method or it seems it could end up being uncaught in the Completion handler when AsynchronousChannelGroupImpl executes this as a task on a ThreadPool.
Previously, the implementation wrapped the callback in errorHandlingCallback(), which did not propagate errors but instead logged them at the ERROR level.
Has the behavior change been intentional? If so, could you please provide some insight into the reasons behind this modification?
There was a problem hiding this comment.
Discussed. Although this is intended to handle only driver bugs (and abuse of throws), we can keep any usages of errorHandlingCallback until we systematically remove them.
|
Looks good overall! Just a couple of questions and suggestions. |
Refactoring for JAVA-5379
Introduces throwTranslatedWriteException (in CSOT will allow replacing isExpired with onExpired).
This also introduces a helper, and the second commit, which can be reviewed independently, is a simple refactoring (move, increase visibility).