Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide additional debug information for memory leaks via ByteBuf.touch [SPR-17427] #21960

Closed
spring-projects-issues opened this issue Oct 24, 2018 · 4 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

spring-projects-issues commented Oct 24, 2018

Violeta Georgieva opened SPR-17427 and commented

Hi,

Netty provides the following API

io.netty.buffer.ByteBuf.touch()


    /**
     * Records the current access location of this object for debugging purposes.
     * If this object is determined to be leaked, the information recorded by this operation will be provided to you
     * via {@link ResourceLeakDetector}.  This method is a shortcut to {@link #touch(Object) touch(null)}.
     */
    ReferenceCounted touch();

When the implementation does additional transformations of the content or buffers this object, please use this touch() functionality. If there is a memory leak, when Netty dumps the stack traces of the components that accessed this object, it will be easier to find where the buffer was leaked.

Thanks,
Violeta

 


Affects: 5.1.1

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Oct 26, 2018

Rossen Stoyanchev commented

I've looked into this, and what I want to propose is to insert touch calls inside NettyDataBufferFactory and NettyDataBuffer. There we have access to the native ByteBuf without the need to downcast, and they capture all things that can happen to buffers (read, write, slice, collect, etc) in a general way.

The alternative of interspersing calls to touch above that layer, i.e. in the codecs, is not feasible I think. Take StringDecoder for example. It breaks up each input buffer along delimiters, then creates slices of chunks, buffers them up until each delimiter, then joins chunks into a stream of delimited Strings. For once it would be super noisy to insert calls for all this in StringDecoder, but it is also not possible to capture everything because joining goes into the DataBufferFactory where a composite is created. Likewise many decoders rely on DataBufferUtils#join which once again leads into the DataBufferFactory.

Based on my proposal the touch messages will be generic (e.g. "read", "write", "slice", etc) but since touch captures a stack trace that should show where the operation is coming from. In fact we might as well use the no-arg touch().

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Oct 26, 2018

Rossen Stoyanchev commented

I'm moving to 5.1.3 since we'll need time to agree and experiment with the outcome.

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Nov 2, 2018

Rossen Stoyanchev commented

I've tried inserting calls in operations of NettyDataBufferFactory and NettyDataBuffer, but after experimenting I realized that Netty already does this and I'm only duplicating records. For example AdvancedLeakAwareByteBuf intercepts and delegates every call to ResourceLeakTracker, that is unless "io.netty.leakDetection.acquireAndReleaseOnly" is set to true, which is false by default.

Given this I can't think of anything useful we can do by calling touch() that isn't already covered by the built-in record tracking. Our codecs either call operations on the input ByteBuf right away (which are tracked automatically) or we use buffering operators (e.g. bufferUntil, collectList) which will not release the input buffers anyway for garbage collection until finished, and on the next stage we'll operate on the buffer which again will leave records. So unless I'm missing something, in case of a leak we should already have all the records we can have, based on the way we operate on data buffers where we don't explicitly queue them ourselves.  

 

@spring-projects-issues spring-projects-issues added status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement in: web Issues in web modules (web, webmvc, webflux, websocket) labels Jan 11, 2019
@rstoyanchev rstoyanchev changed the title Provide additional debug information when there is a memory leak (Reactor Netty use case) [SPR-17427] Provide additional debug information for memory leaks via ByteBuf.touch [SPR-17427] Mar 29, 2019
@rstoyanchev rstoyanchev reopened this Mar 29, 2019
@rstoyanchev rstoyanchev removed the status: declined A suggestion or change that we don't feel we should currently apply label Mar 29, 2019
@rstoyanchev rstoyanchev added this to the 5.1.6 milestone Mar 29, 2019
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Mar 29, 2019

Adding a touch call here does make sense:

public NettyDataBuffer wrap(ByteBuf byteBuf) {
return new NettyDataBuffer(byteBuf, this);
}
because we're wrapping the buffer and keeping it.

The constructor however is not suitable because that's also called from places that slice or retain and those Netty already records, so there is no need for us to do it too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants