|
I checked in the fix.
Could you review the fix and close this issue? The code you asked about is semantically the same as the original, it was
inadvertantly left in the patch. So, there's another leak in this same file, which has proven harder to fix.
I think it needs Trustin's brain to figure out the correct fix. For now we have a hack in place that removes the leak but it also removes the nio byte buffer pools too :( The leak is caused by the fact that all nio.ByteBuffer objects are pushed onto a stack when they are released. However, not all nio.ByteBuffer objects used in allocate are pulled off the stacks. Specifically the two wrap() methods just call the underlying java.nio.ByteBuffer wrap() methods. This results in objects being allocated more often than they are freed and more and more memory leaked onto the stacks. I'm not sure if it's just a case of adding code to try to pull a buffer off the stacks that can then wrap the payload, or if we need to mark the wrapper buffers and not push them on the stack. The latter is not a good fix because in our tests 90%+ of all the buffers were allocated via the wrap calls. You're right. If users keep wrapping nio buffers, it becomes a severe memory leak. We'll have to add some property like 'managed' so that users disable MINA from releasing it. WDYT?
I like the idea of a marker. You can then check isManaged() to figure out if you have to handle the release. It's a fairly quick way to fix the problem too. However if you decide to change it in the future then it might not be a backwards compat. change. Anyways it's not like we're 1.0 so ... go for it.
Yeah, I thought of that as a fix. However...what made me pause
was the fact that in invesigating the leaks Elliot instrumented the allocs and releases, and we looked at the resulting counters. In the case we were checking (searching for users), something like 90% of the buffer allocations were of the wrapped type. So I was thinking if we disable pooling for those buffers then what value is the pooling mechanism ? If it were known that we could re-cycle nio buffers that were originally made with a wrap() call, then all would be well: just try to get them from the pool first. However, reading the Sun doc I wasn't confident that the internal behavior is well understood. E.g. for the clear() method they say something like : this is a bit like cleaning out the object but not quite (nothing like knowing exactly what your code does, eh ?!). I added more documentation related with wrapping existing NIO buffers or byte arrays so that users can understand the root cause of this issue. I also provided example code that resolves this issue. I'll fix Apache ASN1 codec instead. Please look at this:
http://svn.apache.org/viewcvs.cgi/directory/network/branches/0.7/src/java/org/apache/mina/common/ByteBuffer.java?rev=171076&view=markup I checked in the fix for Apache ASN1 and updated dependencies of ApacheDS.
http://svn.apache.org/viewcvs.cgi/directory/asn1/trunk/codec/src/java/org/apache/asn1/codec/mina/Asn1CodecEncoder.java?rev=171078&view=markup |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Bug
Closed
Major
Memory Leaks
- Stack[] bufferStacks = buf.isDirect()? directBufferStacks : heapBufferStacks;
- Stack stack = bufferStacks[ getBufferStackIndex( bufferStacks, buf.capacity() ) ];
+ Stack stack;
+ if (buf.isDirect())
+ {
+ stack = directBufferStacks[ getBufferStackIndex( directBufferStacks, buf.capacity() ) ];
+ }
+ else
+ {
+ stack = heapBufferStacks[ getBufferStackIndex( heapBufferStacks, buf.capacity() ) ];
+ }
+
I don't see any differences in this patch fragment IMHO. Any ideas?