feat(plc4j/spi): Add option to synchronously await response from PLC #1163
Conversation
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.
takraj
force-pushed
the
pr/spi-await-response
branch
from
eb6db5c
to
224c16e
Compare
| @@ -92,6 +93,8 @@ interface ContextHandler { | |||
|
|
|||
| void cancel(); | |||
|
|
|||
| void awaitResponse() throws InterruptedException, ExecutionException; | |||
There was a problem hiding this 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?
There was a problem hiding this 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.
| @@ -152,6 +152,11 @@ protected void decode(ChannelHandlerContext channelHandlerContext, T t, List<Obj | |||
| continue; | |||
| } | |||
| // Timeout? | |||
| if (registration.isDone()) { | |||
There was a problem hiding this 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.
There was a problem hiding this 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.
| @@ -231,19 +236,21 @@ public Duration getTimeout() { | |||
| completionCallback.andThen(handler.getPacketConsumer()), | |||
| handler.getOnTimeoutConsumer(), | |||
| handler.getErrorConsumer(), | |||
| handler::confirmHandled, | |||
| handler::confirmError, | |||
| handler::cancel, | |||
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this comment.
Its my bug then and test should belong to me! ;-)
| onTimeoutConsumer.accept(e); | ||
| } | ||
| return timeoutException -> { | ||
| final HandlerRegistration registration = reference.get(); |
There was a problem hiding this comment.
Just a note, syntax sugar makes stack traces harder to follow. Explicit creation of new Consumer was intentional to keep stacktrace more obvious.
There was a problem hiding this comment.
OK, I wasn't aware of that. I'll change it back, and leave a comment.
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.