Make ResourceLeakDetector also emit recent access records for IllegalReferenceCountException #9892
Conversation
…ReferenceCountException
|
Can one of the admins verify this patch? |
| } | ||
|
|
||
| @Override | ||
| public ByteBuf retain(int increment) { | ||
| leak.record(); | ||
| return super.retain(increment); | ||
| try { | ||
| return super.retain(increment); |
There was a problem hiding this comment.
Seems like other operation(like getBytes) also need to catch IllegalReferenceCountException.
| private static final long serialVersionUID = 1374377399979428484L; | ||
|
|
||
| public TrackedIllegalReferenceCountException(String message, String accessRecords) { | ||
| super(message + StringUtil.NEWLINE + accessRecords); |
There was a problem hiding this comment.
Since Record actually a throwable. Should we let Record and TrackedIllegalReferenceCountException constitute a parent-child relationship? like:
TrackedIllegalReferenceCountException.cause = head
head.cause = next
...
next.cause = tail
This means we should let Record not only a private class.
There was a problem hiding this comment.
I am not sure I can follow here. Can you explain what exactly this would buy us ?
There was a problem hiding this comment.
Of course.
I made a simple test to illustrate:
public static void main(String[] args) {
// here is three record.
//
// head.next => second.
// second.next => tail.
Record tail = new Record();
Record second = getBytes(tail);
Record head = getInt(second);
head.initCause(second);
second.initCause(tail);
NullPointerException t = new NullPointerException();
t.initCause(head);
throw t;
}
// mock two record.
private static Record getBytes() {
return new Record();
}
private static Record getInt() {
return new Record();
}
Running the above example, we will get exception information:
Exception in thread "main" java.lang.NullPointerException
at io.netty.util.ResourceLeakDetector.main(ResourceLeakDetector.java:676)
Caused by:
at io.netty.util.ResourceLeakDetector.getInt(ResourceLeakDetector.java:690)
at io.netty.util.ResourceLeakDetector.main(ResourceLeakDetector.java:671)
Caused by:
at io.netty.util.ResourceLeakDetector.getBytes(ResourceLeakDetector.java:686)
at io.netty.util.ResourceLeakDetector.main(ResourceLeakDetector.java:670)
Caused by:
at io.netty.util.ResourceLeakDetector.main(ResourceLeakDetector.java:669)
If I remove t.initCause (head), it only produces:
Exception in thread "main" java.lang.NullPointerException
at io.netty.util.ResourceLeakDetector.main(ResourceLeakDetector.java:676)
The NullPointerException in the example is our TrackedIllegalReferenceCountException.
Note that Record itself is a Throwable. So we can use initCause to associate Record with TrackedIllegalReferenceCountException, so that for users, they can directly obtain Record information from the stack.
There was a problem hiding this comment.
I continue to enhance this example, I make a small modification in the Record # toString method:
Running the above example also, I will get:
Exception in thread "main" java.lang.NullPointerException
at io.netty.util.ResourceLeakDetector.main(ResourceLeakDetector.java:679)
Caused by: record info
at io.netty.util.ResourceLeakDetector.getInt(ResourceLeakDetector.java:693)
at io.netty.util.ResourceLeakDetector.main(ResourceLeakDetector.java:674)
Caused by: record info
at io.netty.util.ResourceLeakDetector.getBytes(ResourceLeakDetector.java:689)
at io.netty.util.ResourceLeakDetector.main(ResourceLeakDetector.java:673)
Caused by: record info
at io.netty.util.ResourceLeakDetector.main(ResourceLeakDetector.java:672)
There was a problem hiding this comment.
Since TrackedIllegalReferenceCountException is an exception, and many times we will not catch it, so when this exception is thrown, it may be a good way to intuitively display the Record information displayed in the stack.
| } | ||
|
|
||
| @Override | ||
| protected void finalize() throws Throwable { |
There was a problem hiding this comment.
Do we need release head when AdvancedResourceLeak will be gced..?
There was a problem hiding this comment.
IMHO this finalize should not be here.
buffer/src/main/java/io/netty/buffer/AdvancedLeakAwareByteBuf.java
Outdated
Show resolved
Hide resolved
|
@netty-bot test this please |
|
@normanmaurer |
|
@carryxyh I will try soon. |
| // means leak has been closed and there is no accessRecord. | ||
| return e; | ||
| } | ||
| return new TrackedIllegalReferenceCountException(e.getMessage(), accessRecord); |
There was a problem hiding this comment.
shouldn't we wrap the original e ?
| @@ -0,0 +1,30 @@ | |||
| /* | |||
| * Copyright 2012 The Netty Project | |||
| private static final long serialVersionUID = 1374377399979428484L; | ||
|
|
||
| public TrackedIllegalReferenceCountException(String message, String accessRecords) { | ||
| super(message + StringUtil.NEWLINE + accessRecords); |
There was a problem hiding this comment.
I am not sure I can follow here. Can you explain what exactly this would buy us ?
| /** | ||
| * A special {@link IllegalReferenceCountException} with the ability to track access records | ||
| */ | ||
| public class TrackedIllegalReferenceCountException extends IllegalReferenceCountException { |
There was a problem hiding this comment.
should we make this package-private for now and final ?
There was a problem hiding this comment.
Since this exception will be used in AdvancedLeakAwareByteBuf, this can't be a package-private class..
| } | ||
|
|
||
| @Override | ||
| protected void finalize() throws Throwable { |
There was a problem hiding this comment.
IMHO this finalize should not be here.
|
Fixed.. Pls check.. |
|
@carl-mastrangelo WDYT ? |
|
What's the state of affairs regarding this patch? We still encounter #9882, which is difficult to fix without the information this patch will provide. |
Motivation:
Let IllegalReferenceCountException report the access log when over-released.
Modification:
Result:
Fixes #9229