apache / plc4x

PLC4X The Industrial IoT adapter
https://plc4x.apache.org/
Apache License 2.0
1.32k stars 418 forks source link

[Bug]: OPC-UA returns multiple identical PlcSubscriptionHandles, one for every subscribed tag #1896

Open hyslopc opened 1 week ago

hyslopc commented 1 week ago

What happened?

Current behavior: An OPC-UA subscription request with N tags will return a collection of N identical PlcSubscriptionHandles. Registering a handler for all of these handles will result in N callbacks for every tag. Registering a handler for just one of these handles and ignoring all of the others will result in one callback for every tag change.

Expected behavior: Registering a handler for every subscription handle returned by a subscription request (as described in the documentation, literally a copy/paste of the example from the PLC4X docs) should result in a single callback to the handler for every tag change, not multiple callbacks. In order to achieve this, either the subscription request should return one single PlcSubscriptionHandle, or N unique PlcSubscriptionHandles.

Example showing the problem:

PlcSubscriptionRequest subReq = opcConnector.subscriptionRequestBuilder()
      .addChangeOfStateTagAddress("Weight", "Weight")
      .addChangeOfStateTagAddress("Jam", "Jam")
      .addChangeOfStateTagAddress("HeightAlarm", "HeightAlarm")
      .build();

Collection<PlcSubscriptionHandle> shs = subReq.execute().get().getSubscriptionHandles();</code>

After the above code is executed, shs now contains 3 identical PlcSubscriptionHandles. If 3 more tags were to be added to the subscription request, then shs would contain 6 identical PlcSubscriptionHandles.

Version

v0.13.0

Programming Languages

Protocols

splatch commented 1 week ago

I am not sure if I fully follow issue, but I think we need to call @chrisdutz to see how it works across other protocols. In principle OPC UA allows to create one subscription with many tags, thus at the protocol level this is just one subscription (single subscription id), and we get updates for all tags through the same handle.

hyslopc commented 1 week ago

Yes I understand. I am not an expert, but to me the solution seems clear: OPC UA should return a single handler. That would make the examples in the documentation work, and would result in PLC-independent code (you always register with all handlers returned).

hyslopc commented 1 week ago

Here is the example from the documentation (from https://plc4x.apache.org/plc4x/0.13.0-SNAPSHOT/users/getting-started/plc4j.html) which gives the wrong result with OPC UA when subscribing to multiple tags:

for (String subscriptionName : response.getFieldNames()) {
    final PlcSubscriptionHandle subscriptionHandle = response.getSubscriptionHandle(subscriptionName);
    subscriptionHandle.register(plcSubscriptionEvent -> {
        for (String tagName : plcSubscriptionEvent.getTagNames()) {
            System.out.println(plcSubscriptionEvent.getPlcValue(tagName));
        }
    });
}
codekundan commented 1 week ago

@hyslopc Are you able to read subscription tags after few hours?

hyslopc commented 1 week ago

It is working for me, but I think there are some problems with subscriptions with the current PLC4X code, the code is polling constantly once per second, which I believe should not be needed. I am working on a bug report regarding those problems, but I want to have a really clear idea as to exactly what is wrong before I file the bug report. I am also experiencing pretty massive memory leaks with the latest v0.13.0 snapshot, which should already be fixed according to another bug report, but haven't had a chance to fully investigate and make a minimal working example of that yet, but I'm working on it.

codekundan commented 1 week ago

@hyslopc I wrote a program where I subscribed to change of value of a tag. It is working quite well but after 1 or 1.5 hours, the event is not responding with values. Can you please share a code sample of how it is working for you even after few hours?

hyslopc commented 1 week ago

If you're getting callbacks for the first hour or so, then I think you're doing everything right and I would strongly suspect the memory leak I am experiencing. Add some code to log used/free heap once per minute, to see if that's the issue.

codekundan commented 1 week ago

@hyslopc Perhaps it can be memory leak. Can't say without proof. But I want to know is there any way I can increase the KeepAlive time of Subscription? I think there is a default time limit set for subscription, beyond that Subscription does not respond. May be @splatch or @chrisdutz or any PLC4X team members can throw some light on this.

hyslopc commented 1 week ago

I don't think there's any time limit on subscriptions, but they are only valid for the lifetime of the channel, and there is a channel-lifetime setting, which defines the maximum amount of time the channel will be open (in fact the server must leave the channel open 25% longer than this). Default is one hour, so after 75 minutes, you will be disconnected, unless you specify a higher channel-lifetime (and your server supports it: you can see in the reply what it accepts, but this is not available to you via the PLC4X API, so probably you will not see it). If your connection to the OPC UA server times out and you reconnect, you need to re-subscribe. This is what I am doing. In fact I am at 74 minutes starting a new connection, resubscribing, and then once that is in place, I close the old session manually, before it times out. Works fine (except for the memory leak).

codekundan commented 1 week ago

Thanks @hyslopc . Perhaps you are correct on this. I should do the same and check.

splatch commented 1 week ago

It is working for me, but I think there are some problems with subscriptions with the current PLC4X code, the code is polling constantly once per second

@codekundan @hyslopc AFAIR you can try to adjust that, you need to use cyclic subscription. Cycle duration used for first tag will dictate cycle time for polling OPC-UA server: https://github.com/apache/plc4x/blob/v0.12.0/plc4j/drivers/opcua/src/main/java/org/apache/plc4x/java/opcua/protocol/OpcuaProtocolLogic.java#L768

If not provided it is assumed to be 1s. OPC-UA protocol itself requires publish request to receive publish response with changed tag values, so you can reduce load, at the cost of bigger delay since possible value change.

I don't think there's any time limit on subscriptions, but they are only valid for the lifetime of the channel, and there is a channel-lifetime setting, which defines the maximum amount of time the channel will be open (in fact the server must leave the channel open 25% longer than this). Default is one hour, so after 75 minutes, you will be disconnected, unless you specify a higher channel-lifetime

Channel lifetime is declared by server, when secure channel is open or renewed. For that reason we do not have a setting for that, we just rely on what server wants. If you suspect any resource leak then renewing of channel might possibly be guilty (AFAIR it was guilty of some thread leak before), other place could be subscription handles which also need to submit asynchronously a publish request, beside your application logic. Look at these two places (OpcSubscriptionHandle.java and SecureChannel.java). If you have a lot of connect/disconnect cycles then both will fail to properly manage threads started within their internals.

hyslopc commented 1 week ago

Channel lifetime is declared by server, when secure channel is open or renewed. For that reason we do not have a setting for that

There is a setting for it, called "channel-lifetime": it's documented at https://plc4x.apache.org/plc4x/0.13.0-SNAPSHOT/users/protocols/opcua.html. Using this setting, I can control the channel lifetime using my OPC UA server, however it will not accept values lower than 10 minutes or higher than 60 minutes, and (as acc to OPC UA spec), it allows 25% margin, so max is 75 minutes, but this will vary from server to server.

splatch commented 1 week ago

This is correct, we take value which is smaller, so you can override the lifetime, but some servers might be unhappy if you submit renew request too early. That's why safest option is to rely on 75% of declared server lifetime. Anyhow, you can safely report issue with memory leaks you observed, by looking at the code we have I can confirm possibility of these.

codekundan commented 1 week ago

@splatch I am OK with 1 sec polling as I don't want to miss any updated value from Subscribed tag. I want to know to Is it possible to subscribe a tag for as long as possible or perhaps till the user closes the Main Application? Will Cyclic Subscription fulfill that requirement?

codekundan commented 1 week ago

It is working for me, but I think there are some problems with subscriptions with the current PLC4X code, the code is polling constantly once per second

@codekundan @hyslopc AFAIR you can try to adjust that, you need to use cyclic subscription. Cycle duration used for first tag will dictate cycle time for polling OPC-UA server: https://github.com/apache/plc4x/blob/v0.12.0/plc4j/drivers/opcua/src/main/java/org/apache/plc4x/java/opcua/protocol/OpcuaProtocolLogic.java#L768

If not provided it is assumed to be 1s. OPC-UA protocol itself requires publish request to receive publish response with changed tag values, so you can reduce load, at the cost of bigger delay since possible value change.

I don't think there's any time limit on subscriptions, but they are only valid for the lifetime of the channel, and there is a channel-lifetime setting, which defines the maximum amount of time the channel will be open (in fact the server must leave the channel open 25% longer than this). Default is one hour, so after 75 minutes, you will be disconnected, unless you specify a higher channel-lifetime

Channel lifetime is declared by server, when secure channel is open or renewed. For that reason we do not have a setting for that, we just rely on what server wants. If you suspect any resource leak then renewing of channel might possibly be guilty (AFAIR it was guilty of some thread leak before), other place could be subscription handles which also need to submit asynchronously a publish request, beside your application logic. Look at these two places (OpcSubscriptionHandle.java and SecureChannel.java). If you have a lot of connect/disconnect cycles then both will fail to properly manage threads started within their internals.

@splatch I will look into the files you mentioned. Will try Cyclic subscription and get back to you.

hyslopc commented 1 week ago

OPC-UA protocol itself requires publish request to receive publish response with changed tag values, so you can reduce load, at the cost of bigger delay since possible value change.

It's correct that OPC-UA requires a publish request to receive a publish response, however it's not correct that a publish request must be sent every second, but I will file a separate bug report about that, so we don't get too off-topic on this one.