Example Java "Listener" code appears to cause memory leaks
Opened this issue · 5 comments
Since there's not an awful lot of Java documentation and tutorials available for this product, I utilised the example code for getting started with Java. One of the key things is of course using callbacks for delivery of data, hence the "Listener" example shown here.
Looking at examples/dcps/Listener/java5/ListenerDataListener.java (180404OSS):
List<Sample<Msg>> samples = new ArrayList<Sample<Msg>>();
reader.select().dataState(ds).read(samples);When read(List<Sample<T>>) or take(List<Sample<T>>) is called, the OpenSplice code looks at a ConcurrentHashMap<List<Sample<T>>, PreAllocatorImpl<T>> named preallocated in DataReaderImpl . If the List<Sample<T>> is not a key in the map, the List<Sample<T>> is added to the map with a new PreAllocatorImpl in DataReaderImpl.getPreAllocator(List<Sample<T>>, Class<?>, Field). If the key already exists in the map, the new samples are added to the List<Sample<T>>. However, since in this case we create a new List<Sample<T>> every time in onDataAvailable(DataAvailableEvent<T>), a new key/value pair is added to the map every time data is delivered and OpenSplice never cleans the map. Memory grows and eventually the application dies.
After discovering this I changed the code to simply read() or take() and iterate over the returned samples.
This still resulted in slow leaks detected in the form of increasing heap usage. Another heap dump this time showed that again DataReaderImpl was retaining a large amount of data this time in AbstractDataReader<T> contained in the protected member variable HashSet<AbstractIterator<T>> named iterators. Firstly, despite the fact this DataReader<T> contains the HashSet<AbstractIterator<T>>, it is the constructor of the AbstractIterator<T> that ends up adding itself to the HashSet contained within the AbstractDataReader<T>. It is worth noting here that despite being a HashSet, neither AbstractIterator<T> or IteratorImpl<T> contain equals() or hashCode() methods.
Again what is unclear from the example is that read() or take() returns an Iterator<T> and this is not a java.util.Iterator but a different type that is a DDS iterator that has a close() method on it (which is not present in java.util.Iterator). It seems this close() method must be called in order to prevent this iterators object growing.
Since the Java reference documentation doesn't really help a lot in the API presented in the OpenSplice package, it seems natural for people to use the example code provided as a starting point. However, this has caused a lot of pain in development here because of subtle issues like this and similar, so I thought it would be worth pointing it out. In the end, the working code looked similar to the following:
Sample.Iterator<T> iterator = status.getSource().select().dataState(status.getSource().getParent().createDataState()
.with(SampleState.NOT_READ)
.with(ViewState.NEW)
.with(ViewState.NOT_NEW)
.withAnyInstanceState()).take();
List<T> samples = new ArrayList<>();
while (iterator.hasNext()) {
samples.add(iterator.next().getData());
}
/* do work with samples
...
*/
try {
iterator.close(); /* this appears to clear the iterator from {@link org.opensplice.dds.sub.AbstractDataReader#iterators} **/
} catch (IOException e) {
logger.warn("Failed to close DDS iterator; this may result in memory leaks", e);
}Dear Rdghickman,
Thank you for your inputs.
You can get Java5 API reference document from below link:
http://download.ist.adlinktech.com/docs/Vortex/apis/ospl/java5/html/index.html
Reading/Taking Samples in Java 5 is pretty simple, highlights are below:
- The Java 5 API simplifies and extends the ways in which data can be read/taken:
- Sample data through getData method
- Sampleinfo as read-only Java-Bean-style properties
- Sample.Iterator which loans samples from the Service pool
5. List which deep copies the data in a provided List
6. Return of the loan is implicit and managed by scoping
Here are the example codes to read/take samples in two different ways:
- Using pre allocated buffer
- Using loaned buffer
Code snipped are below:
**1: Read ALL samples available that represent NEW instances who are still ALIVE using a pre allocated buffer **
DataState ds = subscriber.createDataState();
ds = ds.with(SampleState.NOT_READ)
.with(ViewState.NEW)
.with(InstanceState.ALIVE);
List<Sample> samples = new ArrayList<Sample>();
fooDR.select().dataState(ds).read(samples);
/* Process each Sample. For example print its name and production time */
for (Sample sample : samples) {{
Foo foo = sample.getData();
Time t = sample.getSourceTimestamp();
System.out.println(“Name: ” + foo.myName +
“ is produced at ” + time.getTime(TimeUnit.SECONDS));
**2: Take ALL samples available that represent NEW instances who are still ALIVE using ‘loaned’ buffers **
DataState ds = subscriber.createDataState();
ds = ds.with(SampleState.NOT_READ)
.with(ViewState.NEW)
.with(InstanceState.ALIVE);
Iterator<Sample> samples = fooDR.select().dataState(ds).take();
/* Process each Sample. For example print its name and production time /
while (samples.hasNext()) {
Sample sample = samples.next();
Foo foo = sample.getData();
Time t = sample.getSourceTimestamp();
System.out.println(“Name: ” + foo.myName +
“ is produced at ” + time.getTime(TimeUnit.SECONDS));
}
/ Returning the loan is implicit in Java 5 API */
Since Java 5 API implicitly returns the loaned buffer, there is no chance to memory leak.
I am investigating your reported issue at my end. I will keep you posted.
Hi Richard,
I guess you are using protobuf when running into this issue? Because I don't notice this behavior for normal sample types.
Let me explain a little bit what happens under the hood. The Java5 API is built on top of the classical Java API, and so the read() call on the Java5 API delegates to the read() call of the classical API. Since the Java5 API returns a List<> of samples, whereas the classical Java API return an array of samples and an array of SampleInfo (whereas the representation of the individual samples in Java5 is identical to their representation in the classical Java API), the sample references will need to be copied from the arrays to their corresponding List<> objects, and this happens transparently under the hood for you.
When you want to benefit from preallocating your own samples, you intend to re-use not only the List<> object, but also all samples contained therein. However, since the Java5 API forwards to the classic Java API, you want to not only re-use the samples, but also the arrays used for carry these samples and their SampleInfo in the underlying classical API. And that is exactly the purpose of the HashList you encountered.
You are right that if you use the read()/take() call with the pre-allocated List<>, but you don't intend to recycle that List, that this mechanism will leak every List (including its samples) away. However, that is not what the pre-allocated pattern was intended for. So to get rid of the Leak in your example, you might want to consider using the following alternatives:
- You could hold on to the List<> object in an attribute of the Listener, so that you can pass it back in on the next Listener callback.
- You can use an alternative read()/take() call, that does not require you to pre-allocate your samples. (Take the version that doesn't have any input parameters). That way your List<> object is not cached anywhere and fully garbage collected at the end of every Listener callback, but at the expense of having to reallocate each List<> and each sample object on every Listener callback.
Hope that helps you work around your problem.
Regards,
Erik Hendriks.
Indeed there is a MAP that keeps track of all pre-allocated sample Lists, and their underlying cla
Thanks for the response.
Yes, I follow what's going on now, my point was simply that given what example code was supplied in the package, little of this can be inferred and hence I tripped over these issues and had to dig around myself with little clue from the API or DDS itself as to what was going wrong. For example, in ListenerDataListener.java supplied in the examples, it does create a new List<Sample<Msg>> on every callback to void onDataAvailable(DataAvailableEvent<Msg>), which is precisely what you have suggested not to do (and which I eventually figured out).
I appreciate there is a Javadoc output for the Java 5 API but this also does not really aid in the "correct" use of it at least from a beginner's point of view, which is why I jumped straight into the example code. I am sure it is useful if you already understand how to correctly write code in the DDS paradigm but otherwise it is of far less use.
It's been a bit painful getting started even though I've finally managed to get somewhere. I feel the only reason I've been able to actually progress this far is by picking into the DDS source code and trying to guess what I'd done wrong, which is a little bit suboptimal for a new developer. As such, I was just offering my opinion that perhaps the example code (at least for Java) should have another glance over in order to aid other developers in the future.
Currently we are not using protobuf. At least, all code paths seem to have gone through DataReaderImpl not DataReaderProtobuf. I basically determined all of this through heap dump analysis and picking through the DCPS Java 5 source code. The runtime was OpenJDK 11.0.7/arm32v7 with HDE 6.7 180404OSS.
You are right, this is sort of a poor example on how to use pre-allocated memory. It would be better for this example to use loaned memory instead. I will write a ticket internally to get it changed, but don't know how fast this will be picked up.
Little update to this ticket. In the upcoming 6.11 release the examples have been updated to use loaned memory instead of pre-allocated memory for this particular reading pattern. Also we decided to only keep track of the last pre-allocated batch so if in the future you do use pre-allocation the wrong way, you don't leak away all previous allocations. Instead, the Reader will only keep track of the last pre-allocated batch. This would still be helpful in the typical scenario where you pre-allocate a batch, and then use the same batch over and over again in a while loop to access data as soon as you are notified it comes in.