[Bug]: OPM memory leak

[botteonr]

What happened?

Production use case:

• Constantly reading and writing (every second) from and to a Plc via modbus tcp.

Issue:

• After running for days (or weeks depend on -Xmx) the application crashes because an OutOfMemoryError.

Comparing dumps (from production) show an increase of byte[] objects like:
java.lang.String#45053 : FooRxEntity$ByteBuddy$opFq0T9M$auxiliary$5LGzfXOE.

The ByteBuddy instances are created here:
org.apache.plc4x.java.opm.PlcEntityManager#connect(java.lang.Class<T>, java.lang.String, T)

therefore I think that PlcEntityManager is responsible for the leak.

Code to reproduce the issue:

import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.when;

import org.apache.plc4x.java.DefaultPlcDriverManager;
import org.apache.plc4x.java.api.exceptions.PlcConnectionException;
import org.apache.plc4x.java.api.types.PlcResponseCode;
import org.apache.plc4x.java.mock.connection.MockConnection;
import org.apache.plc4x.java.mock.connection.MockDevice;
import org.apache.plc4x.java.opm.OPMException;
import org.apache.plc4x.java.opm.PlcEntityManager;
import org.apache.plc4x.java.spi.messages.utils.ResponseItem;
import org.apache.plc4x.java.spi.values.PlcSTRING;
import org.junit.jupiter.api.Test;
import org.mockito.Mockito;

public class Plc4jMockConnectionTest {

  @Test
  void shouldRead() throws PlcConnectionException {
    MockDevice mockDevice = Mockito.mock(MockDevice.class);
    DefaultPlcDriverManager driverManager = new DefaultPlcDriverManager();
    MockConnection connection = (MockConnection) driverManager.getConnection("mock:test");
    when(mockDevice.read(any())).thenAnswer(invocation -> new ResponseItem(PlcResponseCode.OK, new PlcSTRING("1")));
    connection.setDevice(mockDevice);

    PlcEntityManager entityManager = new PlcEntityManager(driverManager);

    while (true) {
      try {
        FooRxEntity read = entityManager.read(FooRxEntity.class, "mock:test");
        Thread.sleep(10);
      } catch (OPMException e) {
        throw new RuntimeException(e);
      } catch (InterruptedException e) {
        throw new RuntimeException(e);
      }
    }
  }
}

And the entity:

import org.apache.plc4x.java.opm.PlcEntity;
import org.apache.plc4x.java.opm.PlcTag;

@PlcEntity
public class FooRxEntity {
  @PlcTag("input-register:1:WORD")
  private short bp102;
  @PlcTag("input-register:2:WORD")
  private short bp105;
  @PlcTag("input-register:3:WORD")
  private short bp112;
  @PlcTag("input-register:4:WORD")
  private short bp115;
  @PlcTag("input-register:5:WORD")
  private short bp122;
  @PlcTag("input-register:6:WORD")
  private short bp125;
  @PlcTag("input-register:7:WORD")
  private short bp360;
  @PlcTag("input-register:8:WORD")
  private short bp260;
  @PlcTag("input-register:9:WORD")
  private short bf102;
  @PlcTag("input-register:10:WORD")
  private short bf112;
  @PlcTag("input-register:11:WORD")
  private short bf122;
  @PlcTag("input-register:21:WORD")
  private short bt220;
  @PlcTag("input-register:22:WORD")
  private short bt230;
  @PlcTag("input-register:23:WORD")
  private short bt260;
  @PlcTag("input-register:24:WORD")
  private short bt320;
  @PlcTag("input-register:25:WORD")
  private short bt330;
  @PlcTag("input-register:26:WORD")
  private short bt360;
  @PlcTag("holding-register:2049:WORD")
  private int bf102Setpoint;
  @PlcTag("holding-register:2050:WORD")
  private int bf112Setpoint;
  @PlcTag("holding-register:2051:WORD")
  private int bf122Setpoint;
  @PlcTag("holding-register:2057:WORD")
  private short qm;
}

The test is started with VM options -Xms128m -Xmx128m. After ~4 minutes OOM is thrown.

I can reproduce the OOE with a real connection. This take a lot more time because the plc support a max reading rate of ~300ms.
Real test:

  @Test
  void shouldRead() {
    PlcDriverManager driverManager = new DefaultPlcDriverManager();
    PlcConnectionManager connectionManager = driverManager.getConnectionManager();
    PlcEntityManager entityManager = new PlcEntityManager(connectionManager);

    while (true) {
      try {
        FooRxEntity read = entityManager.read(FooRxEntity.class, "modbus-tcp:tcp://192.168.100.222:502");
        Thread.sleep(300);
      } catch (OPMException e) {
        throw new RuntimeException(e);
      } catch (InterruptedException e) {
        throw new RuntimeException(e);
      }
    }
  }

Mock dump:
heapdump-1733998935197.zip

Production dump:
heapdump-1732691913936.zip

Please let me know if you need more details and thank you in advance.

Version

v0.12.0

Programming Languages

• plc4j
• plc4go
• plc4c
• plc4net

Protocols

• AB-Ethernet
• ADS /AMS
• BACnet/IP
• CANopen
• DeltaV
• DF1
• EtherNet/IP
• Firmata
• KNXnet/IP
• Modbus
• OPC-UA
• S7

[chrisdutz]

The OPM module hasn't seen any serious contribution for many years. This is due to the fact, that the person who built it, no longer is active in the project.

As far as I know, there isn't anyone else really deeply involved in this. I think I was the only one occasionally working on it.

However have I stopped contributing to the project for free ... I would be willing to have a look at this and your other issue, for a donation of at least 60€ to my BuyMeACoffee page https://buymeacoffee.com/christoferu

[chrisdutz]

Hmmm ... so I took your code and ran it against the current 0.13.0-SNAPSHOT and am not seeing any sort of isssue:

But reading again, it seems to be when running for a very long time, right?

[chrisdutz]

Ok ... running it for longer shows the problem:

However from how I look at the code I think calling read repeatedly seems not quite the design-pattern the initial creator had in mind. It looks as if it was intended to re-use the entity and refresh it.

Right now for every call, ByteBuddy creates a new class instance, which is filling up the memory. I assume there's not much that I can do to fix that. However I most probably will be able to figure out how the initial author was planning to use the OPM (And update the documentation). I do see code that is meant for updating the entity properties ... just not sure how to use it ;-)

In that case you would only create one instance of the Entity and that would prevent the class memory from filling up.

[chrisdutz]

Ok ... so I think I've solved the mystery ...

You were using the OPM the wrong way. However I don't blame you, I intuitively would have done it the same way.

It seems that PlcEntityManager.read is a shortcut to read all values of an OPM Entity, but it doesn't keep the entity "live", but disconnects after reading the values and returns a "dead" entity.

If you however call PlcEntityManager.connect you get a "live" entity back and whenever you call the getter of one of the fields, the OPM will transparently fetch the data in the background. You can fine tune, if you access properties often and don't want each access to result in a PLC read, then you can provide a cacheDurationMillis in the @PlcTag annotation.

So if I change your code to the following code, I no longer see the memory usage increase:

 @Test
    void shouldRead() throws PlcConnectionException {
        MockDevice mockDevice = Mockito.mock(MockDevice.class);
        DefaultPlcDriverManager driverManager = new DefaultPlcDriverManager();
        MockConnection connection = (MockConnection) driverManager.getConnection("mock:test");
        when(mockDevice.read(any())).thenAnswer(invocation -> {
            System.out.println("Reading");
            return new DefaultPlcResponseItem<>(PlcResponseCode.OK, new PlcSTRING("1")) {};
        });
        connection.setDevice(mockDevice);

        PlcEntityManager entityManager = new PlcEntityManager(driverManager);

        try {
            FooRxEntity entity = entityManager.connect(FooRxEntity.class, "mock:test");
            while (true) {
                try {
                    System.out.println(entity.getBf102Setpoint());
                    Thread.sleep(10);
                } catch (InterruptedException e) {
                    throw new RuntimeException(e);
                }
            }
        } catch (OPMException e) {
        }
    }

I am currently updating the documentation on the OPM.

[chrisdutz]

I also updated the entity manager code a bit and now, by adding this method to your FooRxEntity class:

    public void updateAllTheTags() {
        // Dummy ...
    }

You can trigger an update of all properties of the entity.

I would reccomend you add that method to your entity, then you add a sensible cacheDurationMillis value to each @PlcTag annotation. Then instead of calling "entityManager.read" you do the "connect" and in your look you simply call "entity.updateAllTheTags()" to force a bulk read. If all goes right, you should be utilizing the new modbus optimizer to do a lot more efficient bulk reads and then you simply use the entity.

So I just pushed my changes ... there's one code-change needed to make the "updateAllTheTags" thing work, so be sure to pull the latest changes.

I hope I was able to help solve your problem? If not, ping me and I'll do my best to help.

[chrisdutz]

But admittedly I would strongly encourage you to not use the OPM path if you want to fetch many fields repeatedly. Using the PlcConnectionCache (https://plc4x.apache.org/plc4x/latest/users/tools/connection-cache.html) in combination with the normal PLC4X Read API (https://plc4x.apache.org/plc4x/latest/users/getting-started/plc4j.html)

I personally never considered the OPM more than a proof of concept and considering that nobody else seems to have been able to help you and that there's nobody else maintaining this part of the project ... and considering that I personally would probably rather drop the component because cleaning it up and making it a stable solution, would require almost a complete re-write.

I still hope I was able to provide a solution to your problem.

[chrisdutz]

Ok ... took a bit longer than expected ... but the updated documentation should be available here: https://plc4x.apache.org/plc4x/pre-release/users/tools/opm.html

[JoelGaechter]

Thanks a lot for the effort.

The intention was to pass the disconnected entity forward, that's why we used PlcEntityManager::read. To achieve this, the way to go would therefore be to have a connected entity with a copy/clone method. This should have the same effect as updateAllTheTags but also return a copy of the entity. Perhaps this even works with Object::clone, but I would need to verify this for the ByteBuddy proxy.
As suggested, we will probably swap to the the Read API though.

@chrisdutz I think there is a typo in the new documentation. In the Simple Example (Detached) code snippet, it should be read instead of connect I suppose.

[chrisdutz]

Oh yeah ... thanks for spotting that .... should be fixed in a few minutes as soon as the site-build is through.

I think having a connected entity, cloning that and passing it along can cause really odd problems.
Because in this case, every access to a property will cause accessing the PLC via the connection.
Or are you referring to doing a detached read and then cloning the entity? That should work as long as you don't fetch too often, as every fetch costs a dynamic class instance on the class memory.

I would really strongly encourage you not to do the first and still not do to the second option.
In general I would really like to deprecate the whole OPM part of plc4x.

Why don't you simply do a read operation via the PlcConnectionCache and assemble a Pojo from the results and then pass that around? I'd be happy to provide you with an example, if you give me a bit more detailed example (can send me via email directly, if you don't want to share publically)

Chris

[JoelGaechter]

My idea was to obtain a detached copy by cloning the connected entity to avoid further PLC queries. But as said, I will switch to the Read API completely. Either by assembling the entity or directly passing the PlcReadResponse.

[chrisdutz]

I do think a connected entity always calls the PLC on every access to the property (unless the cache setting tells it to use the cached version). So yeah ... please do a regular PlcReadRequest ... you'll have a lot less problems ;-)