You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@plc4x.apache.org by "takraj (via GitHub)" <gi...@apache.org> on 2023/10/18 20:21:30 UTC
[PR] feat(plc4j/spi): Add option to synchronously await response from PLC
takraj opened a new pull request, #1163:
URL: https://github.com/apache/plc4x/pull/1163
There might be cases when the driver needs to wait until the device responds our last request, before sending the next request. This feature attempts to make this more convenient.
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(plc4j/spi): Add option to synchronously await response from PLC
Posted by "takraj (via GitHub)" <gi...@apache.org>.
takraj commented on code in PR #1163:
URL: https://github.com/apache/plc4x/pull/1163#discussion_r1365407396
##########
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:
Yes, I think you are right, these two could be merged together. Moreover, if `isCancelled()` is `true`, `isDone()` is also `true`, so it would be already enough to check the latter.
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(plc4j/spi): Add option to synchronously await response from PLC
Posted by "takraj (via GitHub)" <gi...@apache.org>.
takraj commented on code in PR #1163:
URL: https://github.com/apache/plc4x/pull/1163#discussion_r1365367377
##########
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:
OK, I wasn't aware of that. I'll change it back, and leave a comment.
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(plc4j/spi): Add option to synchronously await response from PLC
Posted by "splatch (via GitHub)" <gi...@apache.org>.
splatch commented on code in PR #1163:
URL: https://github.com/apache/plc4x/pull/1163#discussion_r1365448891
##########
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:
Its my bug then and test should belong to me! ;-)
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(plc4j/spi): Add option to synchronously await response from PLC
Posted by "sruehl (via GitHub)" <gi...@apache.org>.
sruehl merged PR #1163:
URL: https://github.com/apache/plc4x/pull/1163
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(plc4j/spi): Add option to synchronously await response from PLC
Posted by "takraj (via GitHub)" <gi...@apache.org>.
takraj commented on code in PR #1163:
URL: https://github.com/apache/plc4x/pull/1163#discussion_r1365403231
##########
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:
Yea, it's pretty much like a regular `Future`. The implementor `DefaultContextHandler` is basically only a proxy of another `Future` (i.e. `HandlerRegistration`), but seems to also trigger a callback, when cancelled. I'm not sure if we can get rid of that... But still, it could simply implement the normal `Future<Void>` interface instead of this one, and could be renamed to something like `CancellationObserverFuture` (?).
Did you think of something like this?
It would be even better if this `Future` could contain the response, and would `completeExceptionally()` in case of error/timeout. So instead of specifying a callback for these events, one could wait on the future, and then continue with processing the response.
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(plc4j/spi): Add option to synchronously await response from PLC
Posted by "splatch (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(plc4j/spi): Add option to synchronously await response from PLC
Posted by "takraj (via GitHub)" <gi...@apache.org>.
takraj commented on code in PR #1163:
URL: https://github.com/apache/plc4x/pull/1163#discussion_r1365421995
##########
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:
Here, a new `HandlerRegistration` is created, copying the original, in order to inject a `completionCallback` before calling the `packetConsumer`. As `confirmHandled()` and `cancel()` are called for this new `HandlerRegistration`, the original one never became done, so `ConversationContext.isDone()` has never returned `true`.
I'll try to add test for this.
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org