PLC4X-207 When a Handler Timeout occurs cancel the read future to not… #170
Conversation
|
This looks good. It would be nice if there were some tests for this though +1 |
|
@ottobackwards I agree with you (I also would like to have a test), yet I'm unsure how we can provide a Test with lower complexity then the implementation. Perhaps @chrisdutz has an idea with his setup and the test suite? |
| */ | ||
| @Override | ||
| protected void decode(ConversationContext<TPKTPacket> context, TPKTPacket msg) throws Exception { | ||
| throw new IllegalStateException("This should not happen!"); |
There was a problem hiding this comment.
Well it actually could happen ... yesterday I noticed that error responses would go here ... but it's a good place for starting to implement some sort of error handling.
There was a problem hiding this comment.
@chrisdutz yes. BUT the issue is that we want to notify people waiting on the response which is another thread which waits for a future.
One slution would be to have a set of all open futures for this connection and cancel all of them if this method is called ("undefined behavior"). I took the other way and notify the request / reply pattern when a connection si dropped (but nonetheless the paket will then go there).
There was a problem hiding this comment.
I wasn't objecting ... I hope it didn't appear that way ... was more a pointer for myself to continue here to implement correct handling of errors.
There was a problem hiding this comment.
Just today did I push some changes that should help greatly with the decoding of errors in S7 ... the main problem was, that in case of an error, the response is not a S7MessageResponseData type, but a S7MessageResponse (without Data) ... I changed the code to wait for a S7Message with the given PDU Id and to handle that then ... so I think most bad errors were due to this ... perhaps the problem now will go away.
Hi ... I think you should have a look at the XML-base IT framework ... here I can add delays which would be a perfect match for implementing such a test. Please check out: |
|
So right now I think we could probably add a test ... I do not want to force Julian to dig into my IT framework ... if you could explain the situation in which this code would kick in, I could probably come up with a testcase. |
|
I actually re-implemented your fixes in #203 . :-/ |
|
I've pushed changes I made for other PR hence this one needs to be rebased/updated. |
splatch
force-pushed
the
feature/PLC4X-207-fix-silent-exception
branch
from
e028de5
to
01f83be
Compare
|
I've updated code so it works with recent develop. Now S7 driver should properly handle timeouts. |
plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/readwrite/protocol/S7ProtocolLogic.java
Show resolved
Hide resolved
plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/readwrite/protocol/S7ProtocolLogic.java
Show resolved
Hide resolved
plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/readwrite/protocol/S7ProtocolLogic.java
Show resolved
Hide resolved
…ify the user. Author: Julian Feinauer <j.feinauer@pragmaticminds.de> Signed-off-by: Łukasz Dywicki <luke@code-house.org>
splatch
force-pushed
the
feature/PLC4X-207-fix-silent-exception
branch
from
01f83be
to
d03cafb
Compare
…ify the user.