splatch commented on code in PR #1163:
URL: https://github.com/apache/plc4x/pull/1163#discussion_r1365307319
##########
plc4j/spi/src/main/java/org/apache/plc4x/java/spi/ConversationContext.java:
##########
@@ -92,6 +93,8 @@ interface ContextHandler {
void cancel();
+ void awaitResponse() throws InterruptedException, ExecutionException;
Review Comment:
@takraj I am asking myself if there is a point in making `ContextHandler` a
separate thing, since it gets more and more similar to regular `Future`. I
suppose you need `awaitResponse` in your future patches, wouldn't future or
completable future fit better there?
##########
plc4j/spi/src/main/java/org/apache/plc4x/java/spi/Plc4xNettyWrapper.java:
##########
@@ -231,19 +236,21 @@ public Duration getTimeout() {
completionCallback.andThen(handler.getPacketConsumer()),
handler.getOnTimeoutConsumer(),
handler.getErrorConsumer(),
+ handler::confirmHandled,
+ handler::confirmError,
+ handler::cancel,
Review Comment:
I'm trying to grasp with above three lines, cause I believe they are needed,
but I would like to fully understand why they are added. Are we missing error
callback on driver side? Anyhow, would be good if you could leave a comment in
this place. If you spotted a bug then it would be great to have a test which
will prevent us from getting regressions.
##########
plc4j/spi/src/main/java/org/apache/plc4x/java/spi/Plc4xNettyWrapper.java:
##########
@@ -231,19 +236,21 @@ public Duration getTimeout() {
completionCallback.andThen(handler.getPacketConsumer()),
handler.getOnTimeoutConsumer(),
handler.getErrorConsumer(),
+ handler::confirmHandled,
+ handler::confirmError,
+ handler::cancel,
handler.getTimeout()
);
deferred.set(registration);
registeredHandlers.add(registration);
}
private Consumer<TimeoutException>
onTimeout(AtomicReference<HandlerRegistration> reference,
Consumer<TimeoutException> onTimeoutConsumer) {
- return new Consumer<TimeoutException>() {
- @Override
- public void accept(TimeoutException e) {
- registeredHandlers.remove(reference.get());
- onTimeoutConsumer.accept(e);
- }
+ return timeoutException -> {
+ final HandlerRegistration registration = reference.get();
Review Comment:
Just a note, syntax sugar makes stack traces harder to follow. Explicit
creation of `new Consumer` was intentional to keep stacktrace more obvious.
##########
plc4j/spi/src/main/java/org/apache/plc4x/java/spi/Plc4xNettyWrapper.java:
##########
@@ -152,6 +152,11 @@ protected void decode(ChannelHandlerContext
channelHandlerContext, T t, List<Obj
continue;
}
// Timeout?
+ if (registration.isDone()) {
Review Comment:
There is earlier condition which indicates that registration is cancelled.
Maybe making a switch to `isActive` or similar would let us merge both? From
`Plc4xNettyWrapper` perspective it doesn't matter if handler is cancelled or
timed out, its relevant if its still needed to be looped over. With changes you
did I think we got cancel and done callbacks within registration where
appropriate log statements can be made.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
