Marker array memory leak fixes #303
Conversation
…nto marker-array-memory-leak-fixes
|
I tested these changes with my suggested fix and it helped us to solve the memory leak (at least for our use case). It would be great if this will be merged soon. |
Co-Authored-By: mxlle <[email protected]>
|
@mxlle applied that. |
|
|
||
| ROS3D.MarkerArrayClient.prototype.removeMarker = function(key) { | ||
| var oldNode = this.markers[key]; | ||
| if(!oldNode) { |
There was a problem hiding this comment.
What is the purpose of this if statement?
There was a problem hiding this comment.
Null check to ensure the marker hasn't already been deleted. I had caught some race conditions between the timeout portion of MarkerClient and the stream both triggering removals for the same marker. Calling methods on something that doesn't exist will cause an error. The same race condition could happen here if someone's implementation uses removeMarker with other synchronous triggers or if a timeout feature was added to MarkerArrayClient similarly to MarkerClient. Better safe than sorry.
There was a problem hiding this comment.
It seems like if there are null keys it means some method is doing the wrong thing, not a race condition. In JavaScript callbacks like processMessage run to completion before anything else can happen.
Instead of silently passing over these erroneous null values, it would be responsible to:
- Log the error so it can be traced, because this should not be happening in ros3d itself.
- Remove the null key, so we don't bloat the object keys or keep logging the same error.
There was a problem hiding this comment.
The one in MarkerClient with the timeout option is specifically where I am saying there is a potential in library race condition where there could be 2 triggers on removeMarker for the same key.
Log the error so it can be traced, because this should not be happening in ros3d itself.
This covers the other concern I had which was users implementing their own memory cleanup where I agree a warning would be nice, maybe not an error, though, since it is benign redundancy rather than system breaking.
Remove the null key, so we don't bloat the object keys or keep logging the same error.
This already happens, the property is completely removed from the object by removeMarker via delete(this.markers[key]). Sorry I was vaguely using the term null check when in context it is an undefined property. We can specify that vs generic falsy in the if statement as well if desired.
Either way, let me know with those notes what specifically you want for changes and I'll adjust the branch.
There was a problem hiding this comment.
I see now, this will work.
|
|
||
| ROS3D.MarkerArrayClient.prototype.removeMarker = function(key) { | ||
| var oldNode = this.markers[key]; | ||
| if(!oldNode) { |
There was a problem hiding this comment.
It seems like if there are null keys it means some method is doing the wrong thing, not a race condition. In JavaScript callbacks like processMessage run to completion before anything else can happen.
Instead of silently passing over these erroneous null values, it would be responsible to:
- Log the error so it can be traced, because this should not be happening in ros3d itself.
- Remove the null key, so we don't bloat the object keys or keep logging the same error.
|
|
||
| ROS3D.MarkerArrayClient.prototype.removeMarker = function(key) { | ||
| var oldNode = this.markers[key]; | ||
| if(!oldNode) { |
There was a problem hiding this comment.
I see now, this will work.
Same problem fixed in the previous PR for MarkerClient (Memory leak fixes #302) existed on MarkerArrayClient.
Also fix a race condition caused by the timeout in checkTime if a marker happened to be removed between the timeout creation and callback execution