Undo stack can grow unbounded

[pmouawad]

Hello,
We use your nice project in jmeter.
We had a bug reported which leads to OutOfMemoryError, see this for more details:

• https://issues.apache.org/bugzilla/show_bug.cgi?id=55591

See screenshots of analysis:

[bobbylight]

Thanks, I'll look into this. The undo manager should have a maximum undo limit of 100 items by default; however, I'm wondering if the problem here is that each individual undo is a compound edit, which can grow arbitrarily large if you're making huge amounts of contiguous edits (all on one very long line, etc.).

Is this an application with that kind of behavior, such as a log file viewer, perhaps? In scenarios like that, when data is read-only, you can effectively disable the undo stack like so:

public class MyTextArea extends RSyntaxTextArea {

   // ...

   @Override
   protected RUndoManager createUndoManager() {
      RUndoManager undoManager = super.createUndoManager();
      undoManager.setLimit(0);
      return undoManager;
   }

}

[pmouawad]

Thanks for quick reply.
I agree with you it is not related to undo limit (which is set to 100 => reasonable).
It is related to big CompoundEdit blocks.
The use case is the following:

• RSyntaxTextArea holds a String which can be big
• I edit the content and remove blocks of around 1000 lines and input new short text

Removing the undo is not a good option as I find it really useful in this case, but if it's the only option I will do.
Thanks

[pmouawad]

Oups closed by mistake

[bobbylight]

What would be a good "fix" for this? The only thing I can think of is something like this: when a new edit comes in:

• Should it be the start of a new compound edit? If so, proceed as we have previously.
• If not, add it to the current compound edit. If it's size (calculated how?) is over a certain threshold, then start a new compound edit rather than appending to the existing one.

This would cause some edits that would typically be grouped together to be split apart if "too much" content was modified at a specific location in the document. But the net result would be that individual compound edits would no longer grow to be arbitrarily large.

Any other ideas?

Another possible workaround for you would be to set the undo limit to say 30 instead of 100. That way undo is still available, you just can't go back as far in the history.

[danarmstrong]

A couple of ideas that may or may not work in this situation. You could compress the contents in the undo stack so they take up less space or you could use temporary files to store the contents and remove them when the undo manager is destroyed.

As you know graphics editors maintain massive amounts of data in the undo stack and in many cases they resort to the disk since it generally has more space to offer than memory does.

[bobbylight]

UndoableEdits don't have a way to query for their size, so it's impossible to know at runtime just how much memory an individual CompoundEdit takes up. This means we can't write code to simply cap each individual CompoundEdit.

I personally won't write code to compress the undo stack or use disk space as a cache. The former would basically involve doing an optimized serializing/deserializing, but again, there's no way to really know if this work is necessary at runtime (and it typically isn't) since we don't know how much memory the undo stack is actually taking up. Doing it all the time would slow things down for no reason and be error prone. Using temporary files has the same problem - when do you start using them? Also, what if disk access fails for some reason (unlikely of course, but still...).

If someone wants to contribute a patch for this issue, I'd be happy to examine it, but I won't be working on this myself. I suggest simply limiting your undo stack to a smaller size as outlined above to mitigate the issue.

[pmouawad]

Hi,
We have a bug report on JMeter:
https://issues.apache.org/bugzilla/show_bug.cgi?id=57440
In JMeter LoggerPanel we want to disable Undo so we have overriden createUndoManager as per your tip:
https://github.com/apache/jmeter/blob/trunk/src/core/org/apache/jmeter/gui/util/JSyntaxTextArea.java
But this doesn't work as createUndoManager is called before disableUndo is initialized to true.
And unfortunately we cannot access undoManager from our subclass.
I see discardAllEdits() recreates undoManager, so:
1/ is it a reasonable fix to call discardAllEdits() in JSyntaxtTextArea constructor?
2/ or is there a better method
3/ Or could you make undoManager accessible to subclasses through a protected getter ?

Thanks for help
Regards
Philippe M.

[pmouawad]

But if I call discardAllEdits(), I am not sure fix would survive upgrades as I see this.:
/*
* NOTE: For some reason, it appears I have to create an entirely new
* undoManager for undo/redo to continue functioning
* properly; if I don't, it only ever lets you do one undo. Not
* too sure why this is...
*/
public void discardAllEdits() {
undoManager.discardAllEdits();
getDocument().removeUndoableEditListener(undoManager);
undoManager = createUndoManager();
getDocument().addUndoableEditListener(undoManager);
undoManager.updateActions();
}

Another option would be in our createUndoManager() to retain a reference on undoManager and in constructor call undoManager#setLimit .
But those options do not look clean to me :-(

Maybe 3/ is the cleanest one.

[bobbylight]

New suggested workarounds at #99.