Memory leak when deserializing string members

[martins-mozeiko]

If message has member with string type the unserialization code will leak memory. It will be freed only when client unsubscribes or disconnects.

Here's an example that reproduces this: https://gist.github.com/martins-mozeiko/7cc78cc65afe323997965e871b30dc6d

talker.py publishes std_msgs/Header message (with frame_id string set) 1000x a second.
When client (from listener.html) is connected, and you'll run command that monitors memory usage rosnode process you'll see that increases very fast (see gist for example output).

The happens in following method of HeaderWrapper from generated std_msgs__msg__Header.js file:

deserialize(refObject) {                                                                                         
  this._wrapperFields.stamp.copyRefObject(refObject.stamp);                                                      
  this._frame_idIntialized = true;                                                                               
  this._wrapperFields.frame_id.data = refObject.frame_id.data;                                                 
  }

If you comment out frame_id.data assignment, the huge memory increase disappears.
Alternatively you can put following line before frame_id.data assignment:

  this._wrapperFields.frame_id.refObject["ref.buffer"]["_refs"] = [];

Of course this is not a proper fix, this is hacky workaround around the leak.

The issue is following:

1. Subscription class maintains permanently allocated message for each subscription

2. This message contains refObject which is StructType, for string type it is here.

3. First time it is created, it calls primitiveTypes.initString where it allocates 1 byte buffer with Buffer.alloc
   This line assigns newly allocated Buffer to data member. Data member is special member of StructType. It calls writePointer. This function calls exports._attach. _attach function makes sure to keep reference of Buffer while no other JS object is holding reference to it - because only thing now left is raw pointer written into buffer.

4. Whenever new message is incoming for subscription, new message object is allocated and passed to rclTake. It calls to toRawROS() function which calls freeze(true) which makes string type to call primitiveTypes.initString and allocate memory with call to ROS2 C function rosidl_generator_c__String__init

5. After returning from rclTake it eventually calls deserialize function (posted above in beginning of this issue) from Subscription.processMessage

6. Now deserialize function assigns data from refObject.frame_id message (where string is allocated with C malloc in ROS2 C code) to data of this._wrapperFields.frame_id that is allocated to one "global-per-Subscription" message. This is done by writing pointer of Buffer (from refObject.frame_id) to this._wrapperFields.frame_id structure. This structure makes StructType of this._wrapperFields.frame_id to call writePointer with Buffer from refObject.frame_id. So same writePointer method is called as in step 3, so it appends reference to newly allocated Buffer (from step 4) to _refs array (allocated in step 3).

7. After deserialize finishes and message is passed further down to websocket for sending to client it is destroyed by calling freeStruct
   freeStruct correctly releases memory of actual string (by calling free on data member of String struct), but Buffer that keeps whole String struct is left in _refs array.

Now repeat steps 3..7 many times and you can see how _refs array will grow in size. It will be full of Buffer objects. You can verify this by putting console.log in deserialize function and make it print the length of _refs array. You'll see it is increasing, but actually it should never contain more than 1 element for String type. This is the leak.

And because std_msgs/Header is a very common type - it appears in many other messages, this memory leak will happen for a lot of other messages.

I tried removing Subscription._message member from constructor and simply allocate new message object inside Subscription.processMessagefunction every time it is called. Then I would need to release message object with MessageTypeClass.destroyRawROS(msg), right? But this does not work obviously, because both messages will try to call free for C string pointer two times and node will crash.

Sorry for long text, I wanted to be sure I'm not missing anything.
Any ideas on proper fix?

[minggangw]

Hi @martins-mozeiko It's grateful that you did such a deep and detailed investigation about this memory usage issue, and the information is enough to identify the defect.

The process of receiving a topic in a subscription is just as you analysed above. Because the rcl functions, which were wrapped by the rclnodejs, will allocate memory when receiving a topic, the rclnodejs has to deallocate it manually later which makes the code a little bit weird and confused. For JavaScript lanugage, we usually do not need to free a memory explicitly, as the GC takes care of it.

In this case, we do not miss a block of memory to be freed actually. I did not realize before that the ref pushes the object to its internal array, _refs, to prevent it from being garbage collected when assigning a new value . I think this is the root cause of keeping the memory increased.

The method you have tried was almost right I think, I wrote it blow (subscription.js):

  processResponse(msg) {
    this._message = new this._typeClass();
    this._message.deserialize(msg);
    debug(`Message of topic ${this._topic} received.`);
    this._callback(this._message.toPlainObject());
  }

The C string you mentioned should be deallocated by freeStructMember in Node class.

I tested with it and the memory consumption stabilized at a certain level, would you please double check it? If that corrects the problem, there are the same issues for client/service. Then we can submit a PR to fix them simultaneously.

[martins-mozeiko]

Ah, I see. So if message object is not passed to rclnode native code it does not need to call destroyRawROS, because all memory allocations happens in JS and GC will take care of them.

I changed processResponse to create message as local variable instead of assigning it to property of this.

processResponse(msg) {
  let message = new this._typeClass();
  message.deserialize(msg);
  debug(`Message of topic ${this._topic} received.`);
  this._callback(message.toPlainObject());
}

After running patched code for ~4 hours I see no memory increase. It stays more or less the same except fluctuations due GC.

Thanks!

[martins-mozeiko]

Btw, I have not used services, but it seems similar change needs to happen also in service.js

[minggangw]

Ah, I see. So if message object is not passed to rclnode native code it does not need to call destroyRawROS, because all memory allocations happens in JS and GC will take care of them.

@martins-mozeiko your understanding is right, if we didn't call the functions of rcl library, then we don't need to deallocate any memory on heap.

Btw, I have not used services, but it seems similar change needs to happen also in service.js

The same issues also exist in client/service when receiving response/request, and I will fix them all ASAP.

@qiuzhong @hanyia @Christywl the old issue #175 which reported the memory usage increasing may have been found the root cause, I suggest we test it when the PR is submitted.

Thanks again for @martins-mozeiko investigation!

[minggangw]

@martins-mozeiko thanks for your special contribution, I have added you as a contributor in README. We will continue to improve the rclnodejs in robustness and performance and you are always welcome to report any issue you find and contribute your code, thanks!

[minggangw]

One more thing, we will have our next release to be published on npmjs this week and you will not have to patch locally. 😄

[martins-mozeiko]

Thanks! That will be very useful.