You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@plc4x.apache.org by GitBox <gi...@apache.org> on 2020/07/08 19:48:16 UTC
[GitHub] [plc4x] JulianFeinauer opened a new pull request #170: PLC4X-207 When a Handler Timeout occurs cancel the read future to not…
JulianFeinauer opened a new pull request #170:
URL: https://github.com/apache/plc4x/pull/170
…ify the user.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] splatch merged pull request #170: PLC4X-207 When a Handler Timeout occurs cancel the read future to not…
Posted by GitBox <gi...@apache.org>.
splatch merged pull request #170:
URL: https://github.com/apache/plc4x/pull/170
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] JulianFeinauer commented on pull request #170: PLC4X-207 When a Handler Timeout occurs cancel the read future to not…
Posted by GitBox <gi...@apache.org>.
JulianFeinauer commented on pull request #170:
URL: https://github.com/apache/plc4x/pull/170#issuecomment-655942698
@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?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on a change in pull request #170: PLC4X-207 When a Handler Timeout occurs cancel the read future to not…
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on a change in pull request #170:
URL: https://github.com/apache/plc4x/pull/170#discussion_r481211814
##########
File path: plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/readwrite/protocol/S7ProtocolLogic.java
##########
@@ -299,6 +319,14 @@ public void onConnect(ConversationContext<TPKTPacket> context) {
return future;
}
+ /**
+ * This method is only called when there is no Response Handler.
+ */
+ @Override
+ protected void decode(ConversationContext<TPKTPacket> context, TPKTPacket msg) throws Exception {
+ throw new IllegalStateException("This should not happen!");
Review 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on a change in pull request #170: PLC4X-207 When a Handler Timeout occurs cancel the read future to not…
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on a change in pull request #170:
URL: https://github.com/apache/plc4x/pull/170#discussion_r543274480
##########
File path: plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/readwrite/protocol/S7ProtocolLogic.java
##########
@@ -69,7 +72,7 @@
*/
public class S7ProtocolLogic extends Plc4xProtocolBase<TPKTPacket> {
- private static final Logger LOGGER = LoggerFactory.getLogger(S7ProtocolLogic.class);
Review comment:
as it's a static, I would prefer the LOGGER ... or make it non-static, then use the lowercase version.
##########
File path: plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/readwrite/protocol/S7ProtocolLogic.java
##########
@@ -91,35 +94,50 @@ public void setDriverContext(DriverContext driverContext) {
@Override
public void onConnect(ConversationContext<TPKTPacket> context) {
+ if (context.isPassive()) {
+ logger.info("S7 Driver running in PASSIVE mode.");
+ s7DriverContext.setPassiveMode(true);
+ // No login required, just confirm that we're connected.
+ context.fireConnected();
+ return;
+ }
+
// Only the TCP transport supports login.
- if(!context.isPassive()) {
Review comment:
Why remove this?
##########
File path: plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/readwrite/protocol/S7ProtocolLogic.java
##########
@@ -136,32 +154,27 @@ public void onConnect(ConversationContext<TPKTPacket> context) {
return;
}
- // Prepare a message to request the remote to identify itself.
- LOGGER.debug("Sending S7 Identification Request");
- TPKTPacket tpktPacket = createIdentifyRemoteMessage();
- context.sendRequest(tpktPacket)
- .expectResponse(TPKTPacket.class, REQUEST_TIMEOUT)
- .check(p -> p.getPayload() instanceof COTPPacketData)
- .unwrap(p -> ((COTPPacketData) p.getPayload()))
- .check(p -> p.getPayload() instanceof S7MessageUserData)
- .unwrap(p -> ((S7MessageUserData) p.getPayload()))
- .check(p -> p.getPayload() instanceof S7PayloadUserData)
- .handle(messageUserData -> {
- LOGGER.debug("Got S7 Identification Response");
- S7PayloadUserData payloadUserData = (S7PayloadUserData) messageUserData.getPayload();
- extractControllerTypeAndFireConnected(context, payloadUserData);
- });
- });
- });
- }
- // This usually when we're running a passive mode river.
- else {
- LOGGER.info("S7 Driver running in PASSIVE mode.");
- s7DriverContext.setPassiveMode(true);
-
- // No login required, just confirm that we're connected.
- context.fireConnected();
- }
Review comment:
We should keep this as we are planning on making the drivers passive capable.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] splatch commented on pull request #170: PLC4X-207 When a Handler Timeout occurs cancel the read future to not…
Posted by GitBox <gi...@apache.org>.
splatch commented on pull request #170:
URL: https://github.com/apache/plc4x/pull/170#issuecomment-723515410
I actually re-implemented your fixes in #203 . :-/
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] ottobackwards commented on pull request #170: PLC4X-207 When a Handler Timeout occurs cancel the read future to not…
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on pull request #170:
URL: https://github.com/apache/plc4x/pull/170#issuecomment-655750717
This looks good. It would be nice if there were some tests for this though
+1
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] splatch commented on a change in pull request #170: PLC4X-207 When a Handler Timeout occurs cancel the read future to not…
Posted by GitBox <gi...@apache.org>.
splatch commented on a change in pull request #170:
URL: https://github.com/apache/plc4x/pull/170#discussion_r543316951
##########
File path: plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/readwrite/protocol/S7ProtocolLogic.java
##########
@@ -136,32 +154,27 @@ public void onConnect(ConversationContext<TPKTPacket> context) {
return;
}
- // Prepare a message to request the remote to identify itself.
- LOGGER.debug("Sending S7 Identification Request");
- TPKTPacket tpktPacket = createIdentifyRemoteMessage();
- context.sendRequest(tpktPacket)
- .expectResponse(TPKTPacket.class, REQUEST_TIMEOUT)
- .check(p -> p.getPayload() instanceof COTPPacketData)
- .unwrap(p -> ((COTPPacketData) p.getPayload()))
- .check(p -> p.getPayload() instanceof S7MessageUserData)
- .unwrap(p -> ((S7MessageUserData) p.getPayload()))
- .check(p -> p.getPayload() instanceof S7PayloadUserData)
- .handle(messageUserData -> {
- LOGGER.debug("Got S7 Identification Response");
- S7PayloadUserData payloadUserData = (S7PayloadUserData) messageUserData.getPayload();
- extractControllerTypeAndFireConnected(context, payloadUserData);
- });
- });
- });
- }
- // This usually when we're running a passive mode river.
- else {
- LOGGER.info("S7 Driver running in PASSIVE mode.");
- s7DriverContext.setPassiveMode(true);
-
- // No login required, just confirm that we're connected.
- context.fireConnected();
- }
Review comment:
It is moved up in the method body so we do not have bigger block with active connection under if brackets.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] JulianFeinauer commented on pull request #170: PLC4X-207 When a Handler Timeout occurs cancel the read future to not…
Posted by GitBox <gi...@apache.org>.
JulianFeinauer commented on pull request #170:
URL: https://github.com/apache/plc4x/pull/170#issuecomment-772385259
@splatch can this PR be closed and the branch be deletet? I think you did all that already in #203 , right?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] splatch commented on pull request #170: PLC4X-207 When a Handler Timeout occurs cancel the read future to not…
Posted by GitBox <gi...@apache.org>.
splatch commented on pull request #170:
URL: https://github.com/apache/plc4x/pull/170#issuecomment-725745760
I've pushed changes I made for other PR hence this one needs to be rebased/updated.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] JulianFeinauer commented on a change in pull request #170: PLC4X-207 When a Handler Timeout occurs cancel the read future to not…
Posted by GitBox <gi...@apache.org>.
JulianFeinauer commented on a change in pull request #170:
URL: https://github.com/apache/plc4x/pull/170#discussion_r452019997
##########
File path: plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/readwrite/protocol/S7ProtocolLogic.java
##########
@@ -299,6 +319,14 @@ public void onConnect(ConversationContext<TPKTPacket> context) {
return future;
}
+ /**
+ * This method is only called when there is no Response Handler.
+ */
+ @Override
+ protected void decode(ConversationContext<TPKTPacket> context, TPKTPacket msg) throws Exception {
+ throw new IllegalStateException("This should not happen!");
Review 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).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on a change in pull request #170: PLC4X-207 When a Handler Timeout occurs cancel the read future to not…
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on a change in pull request #170:
URL: https://github.com/apache/plc4x/pull/170#discussion_r452018041
##########
File path: plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/readwrite/protocol/S7ProtocolLogic.java
##########
@@ -299,6 +319,14 @@ public void onConnect(ConversationContext<TPKTPacket> context) {
return future;
}
+ /**
+ * This method is only called when there is no Response Handler.
+ */
+ @Override
+ protected void decode(ConversationContext<TPKTPacket> context, TPKTPacket msg) throws Exception {
+ throw new IllegalStateException("This should not happen!");
Review 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] splatch commented on a change in pull request #170: PLC4X-207 When a Handler Timeout occurs cancel the read future to not…
Posted by GitBox <gi...@apache.org>.
splatch commented on a change in pull request #170:
URL: https://github.com/apache/plc4x/pull/170#discussion_r543316951
##########
File path: plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/readwrite/protocol/S7ProtocolLogic.java
##########
@@ -136,32 +154,27 @@ public void onConnect(ConversationContext<TPKTPacket> context) {
return;
}
- // Prepare a message to request the remote to identify itself.
- LOGGER.debug("Sending S7 Identification Request");
- TPKTPacket tpktPacket = createIdentifyRemoteMessage();
- context.sendRequest(tpktPacket)
- .expectResponse(TPKTPacket.class, REQUEST_TIMEOUT)
- .check(p -> p.getPayload() instanceof COTPPacketData)
- .unwrap(p -> ((COTPPacketData) p.getPayload()))
- .check(p -> p.getPayload() instanceof S7MessageUserData)
- .unwrap(p -> ((S7MessageUserData) p.getPayload()))
- .check(p -> p.getPayload() instanceof S7PayloadUserData)
- .handle(messageUserData -> {
- LOGGER.debug("Got S7 Identification Response");
- S7PayloadUserData payloadUserData = (S7PayloadUserData) messageUserData.getPayload();
- extractControllerTypeAndFireConnected(context, payloadUserData);
- });
- });
- });
- }
- // This usually when we're running a passive mode river.
- else {
- LOGGER.info("S7 Driver running in PASSIVE mode.");
- s7DriverContext.setPassiveMode(true);
-
- // No login required, just confirm that we're connected.
- context.fireConnected();
- }
Review comment:
It is moved up in the method body so we do not have bigger block with active connection under if brackets. See lines 97-104.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] splatch commented on pull request #170: PLC4X-207 When a Handler Timeout occurs cancel the read future to not…
Posted by GitBox <gi...@apache.org>.
splatch commented on pull request #170:
URL: https://github.com/apache/plc4x/pull/170#issuecomment-745236270
I've updated code so it works with recent develop. Now S7 driver should properly handle timeouts.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz edited a comment on pull request #170: PLC4X-207 When a Handler Timeout occurs cancel the read future to not…
Posted by GitBox <gi...@apache.org>.
chrisdutz edited a comment on pull request #170:
URL: https://github.com/apache/plc4x/pull/170#issuecomment-659973093
> @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?
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:
plc4j/drivers/s7/src/test/resources/testsuite/S7DriverIT.xml
Where I would also suggest to add the test to.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] splatch commented on a change in pull request #170: PLC4X-207 When a Handler Timeout occurs cancel the read future to not…
Posted by GitBox <gi...@apache.org>.
splatch commented on a change in pull request #170:
URL: https://github.com/apache/plc4x/pull/170#discussion_r543318546
##########
File path: plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/readwrite/protocol/S7ProtocolLogic.java
##########
@@ -69,7 +72,7 @@
*/
public class S7ProtocolLogic extends Plc4xProtocolBase<TPKTPacket> {
- private static final Logger LOGGER = LoggerFactory.getLogger(S7ProtocolLogic.class);
Review comment:
Got spot, I did not notice it was static. AFAIR slf4j and modern log4j (even 1.2) recommendation is to use final without static classifier.
##########
File path: plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/readwrite/protocol/S7ProtocolLogic.java
##########
@@ -69,7 +72,7 @@
*/
public class S7ProtocolLogic extends Plc4xProtocolBase<TPKTPacket> {
- private static final Logger LOGGER = LoggerFactory.getLogger(S7ProtocolLogic.class);
Review comment:
Good spot, I did not notice it was static. AFAIR slf4j and modern log4j (even 1.2) recommendation is to use final without static classifier.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on pull request #170: PLC4X-207 When a Handler Timeout occurs cancel the read future to not…
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on pull request #170:
URL: https://github.com/apache/plc4x/pull/170#issuecomment-659973093
> @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?
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on pull request #170: PLC4X-207 When a Handler Timeout occurs cancel the read future to not…
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on pull request #170:
URL: https://github.com/apache/plc4x/pull/170#issuecomment-684924829
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] splatch merged pull request #170: PLC4X-207 When a Handler Timeout occurs cancel the read future to not…
Posted by GitBox <gi...@apache.org>.
splatch merged pull request #170:
URL: https://github.com/apache/plc4x/pull/170
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on a change in pull request #170: PLC4X-207 When a Handler Timeout occurs cancel the read future to not…
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on a change in pull request #170:
URL: https://github.com/apache/plc4x/pull/170#discussion_r452032069
##########
File path: plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/readwrite/protocol/S7ProtocolLogic.java
##########
@@ -299,6 +319,14 @@ public void onConnect(ConversationContext<TPKTPacket> context) {
return future;
}
+ /**
+ * This method is only called when there is no Response Handler.
+ */
+ @Override
+ protected void decode(ConversationContext<TPKTPacket> context, TPKTPacket msg) throws Exception {
+ throw new IllegalStateException("This should not happen!");
Review 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org