ByteBuffer.wrap((byte[],?) The element of least suprise

• Key: DIRMINA-78
• Type: New Feature
• Status: Closed
• Resolution: Fixed
• Priority: Minor
• Assignee: Trustin Lee
• Reporter: Magnus Naeslund
• Affects Version/s: 0.7.0, 0.7.1, 0.7.2, 0.7.3
• Fix Version/s: 0.7.4

Description

*This is just an discussion, not a bug*

The problem:
When mina wraps (buffers or arrays) external data, it keeps push()ing the nio buffer to the stacks of buffers, but it can't reuse them when wrapping (since they either already exist or was created from nio wrap()).

This could be fixed in two ways:

1) When wrapping, re-use buffers as usual, con: extra copy
2) Have a flag that causes the underlying nio buffer to not be pooled after release

I would rather see minas bytebuffer to copy the data than cause an memory leak.
I've just debugged a project that writes byte arrays (old code, needs to write arrays), and found the memory leak that is (kind of) described in the mina ByteBuffer documentation. I ended up with a unbounded leak of nio HeapByteBuffers.

I know that this is discouraged in the documentation, but I don't like these kind of suprises that you get from using mina without carefully reading the documentation (note that the javadoc warning isn't at the method level).
It's like the close(true) that doesn't flush the buffers before closing the session (I can't understand why anyone wants it the way it is now).

I know copy it's slower, but if you're using wrap(), you'll need to do something like that anyways...
I implemented the copy for byte arrays as an example, because it is the least modification, I'll attach it to this issue.

So after this little rant, what do you think about this suggestion?

Regards,
Magnus Naeslund


Magnus

Comments

[Magnus Naeslund]

Example on solving this, but maybe not the cleanest solution (best might be to not pool it at all)

[Magnus Naeslund]

Maybe i should mention that the patch is totally untested :)

Hi, Magnus.

I agree with current documentation can just let user misuse wrap() methods. But users will still require legacy wrap() methods for performance reason. What do you think about renaming existing wrap methods to 'fastwrap' or something else, and add new wrap() methods that is safer. Any better ideas?

[Magnus Naeslund]

Well, the more performant way of doing this would be to have a flag in DefaultByteBuffer that says wether to push the NIO buffer into the stack pool on release().

So when we're using wrap it would still pool the mina bytebuffer container as we do now, but release() would not shove the NIO buffer back into the pool.

[Magnus Naeslund]

I whipped up a patch for showing how we could still have performance with wrapped but no memory leaks.
Totally untested, but it compiles :)

[Trustin Lee]

It is a good idea to specify to pool or not to pool existing NIO buffers and byte arrays internally. I'll try to apply this idea soon, and get back here when done.

[Trustin Lee]

I added a property called 'pooled' to MINA ByteBuffer. This property is 'true' by default except for the case you created the wrapped buffer. In case of the wrapped ones, it is 'false' by default so that they are not returned to the buffer pool. Of course you can change this bahavior by calling 'setPooled' method.

I mark this issue as 'resolved' because it resolves all issues addressed here by users who have a concern on it.