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.

Sign up for GitHub

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

Jump to bottom

Buffer leak reported in THttpClientDelegate #5481

Closed
whyuan opened this issue Mar 4, 2024 · 7 comments
Closed

Buffer leak reported in THttpClientDelegate #5481

whyuan opened this issue Mar 4, 2024 · 7 comments
Labels
defect
Milestone
1.28.0

Comments

@whyuan
Copy link

whyuan commented Mar 4, 2024 •
edited

When the error com.linecorp.armeria.client.circuitbreaker.FailFastException occurs, occasional ByteBuf leak warnings may appear.

armeria version: 0.9.99
netty version: 4.1.94.Final

[2024-02-23 10:26:32][BattleServer-1] ResourceLeakDetector.reportTracedLeak - LEAK: ByteBuf.release() was not called before it's garbage-collected. See https://netty.io/wiki/reference-counted-objects.html for more information.
Recent access records: 
Created at:
    io.netty.buffer.PooledByteBufAllocator.newDirectBuffer(PooledByteBufAllocator.java:407)
    io.netty.buffer.AbstractByteBufAllocator.directBuffer(AbstractByteBufAllocator.java:188)
    io.netty.buffer.AbstractByteBufAllocator.directBuffer(AbstractByteBufAllocator.java:179)
    io.netty.buffer.AbstractByteBufAllocator.buffer(AbstractByteBufAllocator.java:116)
    com.linecorp.armeria.internal.client.thrift.THttpClientDelegate.execute(THttpClientDelegate.java:113)
    com.linecorp.armeria.internal.client.thrift.THttpClientDelegate.execute(THttpClientDelegate.java:71)
    com.linecorp.armeria.client.metric.AbstractMetricCollectingClient.execute(AbstractMetricCollectingClient.java:51)
    com.linecorp.armeria.client.logging.AbstractLoggingClient.execute(AbstractLoggingClient.java:112)
    com.linecorp.armeria.internal.client.ClientUtil.pushAndExecute(ClientUtil.java:150)
    com.linecorp.armeria.internal.client.ClientUtil.initContextAndExecuteWithFallback(ClientUtil.java:95)
    com.linecorp.armeria.internal.client.ClientUtil.initContextAndExecuteWithFallback(ClientUtil.java:67)
    com.linecorp.armeria.client.UserClient.execute(UserClient.java:167)
    com.linecorp.armeria.client.UserClient.execute(UserClient.java:133)
    com.linecorp.armeria.internal.client.thrift.DefaultTHttpClient.execute0(DefaultTHttpClient.java:67)
    com.linecorp.armeria.internal.client.thrift.DefaultTHttpClient.execute(DefaultTHttpClient.java:44)
    com.linecorp.armeria.internal.client.thrift.THttpClientInvocationHandler.invokeClientMethod(THttpClientInvocationHandler.java:143)
    com.linecorp.armeria.internal.client.thrift.THttpClientInvocationHandler.invoke(THttpClientInvocationHandler.java:98)
    jdk.proxy2/jdk.proxy2.$Proxy85.battleMsgNotifyResponse(Unknown Source)
    jdk.internal.reflect.GeneratedMethodAccessor41.invoke(Unknown Source)
    java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    java.base/java.lang.reflect.Method.invoke(Method.java:568)
    rpc.client.ClientPool.invoke0(ClientPool.java:133)
    rpc.client.ClientPool$1.invoke(ClientPool.java:117)
    jdk.proxy2/jdk.proxy2.$Proxy85.battleMsgNotifyResponse(Unknown Source)
    jdk.internal.reflect.GeneratedMethodAccessor41.invoke(Unknown Source)
    java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    java.base/java.lang.reflect.Method.invoke(Method.java:568)
    common.rpc.client.proxy.DefaultRpcProxyFactory$RpcClientProxy.invoke(DefaultRpcProxyFactory.java:80)
    jdk.proxy2/jdk.proxy2.$Proxy85.battleMsgNotifyResponse(Unknown Source)
    battle.service.notify.BattleSubWriter.battleMsgNotifyResponse(BattleSubWriter.java:139)
    battle.service.notify.BattleSubWriter.handleBattleServerResponse(BattleSubWriter.java:110)
    battle.service.notify.BattleSubWriter.doWork(BattleSubWriter.java:67)
    java.base/java.lang.Thread.run(Thread.java:842)
@jrhee17
Copy link
Contributor

jrhee17 commented Mar 5, 2024 •
edited

This looks like a old version, and there's been quite a few bugfixes regarding buffer leak since 😅

Is it difficult for you to try and upgrading and seeing if the issue persists?

@whyuan
Copy link
Author

whyuan commented Mar 9, 2024 •
edited

I tried to upgrade armeria to version 1.27.2，but the issue persists:

ResourceLeakDetector.reportTracedLeak - LEAK: ByteBuf.release() was not called before it's garbage-collected. See https://netty.io/wiki/reference-counted-objects.html for more information.
Recent access records: 
Created at:
    io.netty.buffer.PooledByteBufAllocator.newDirectBuffer(PooledByteBufAllocator.java:404)
    io.netty.buffer.AbstractByteBufAllocator.directBuffer(AbstractByteBufAllocator.java:188)
    io.netty.buffer.AbstractByteBufAllocator.directBuffer(AbstractByteBufAllocator.java:179)
    io.netty.buffer.AbstractByteBufAllocator.buffer(AbstractByteBufAllocator.java:116)
    com.linecorp.armeria.internal.client.thrift.THttpClientDelegate.execute(THttpClientDelegate.java:134)
    com.linecorp.armeria.internal.client.thrift.THttpClientDelegate.execute(THttpClientDelegate.java:78)
    com.linecorp.armeria.client.metric.AbstractMetricCollectingClient.execute(AbstractMetricCollectingClient.java:63)
    com.linecorp.armeria.client.logging.AbstractLoggingClient.execute(AbstractLoggingClient.java:66)
    com.linecorp.armeria.internal.client.ClientUtil.pushAndExecute(ClientUtil.java:160)
    com.linecorp.armeria.internal.client.ClientUtil.initContextAndExecuteWithFallback(ClientUtil.java:114)
    com.linecorp.armeria.internal.client.ClientUtil.initContextAndExecuteWithFallback(ClientUtil.java:80)
    com.linecorp.armeria.client.UserClient.execute(UserClient.java:190)
    com.linecorp.armeria.client.UserClient.execute(UserClient.java:144)
    com.linecorp.armeria.internal.client.thrift.DefaultTHttpClient.execute0(DefaultTHttpClient.java:80)
    com.linecorp.armeria.internal.client.thrift.DefaultTHttpClient.execute(DefaultTHttpClient.java:53)
    com.linecorp.armeria.internal.client.thrift.THttpClientInvocationHandler.invokeClientMethod(THttpClientInvocationHandler.java:142)
    com.linecorp.armeria.internal.client.thrift.THttpClientInvocationHandler.invoke(THttpClientInvocationHandler.java:97)
    jdk.proxy2/jdk.proxy2.$Proxy85.battleMsgNotifyResponse(Unknown Source)
    jdk.internal.reflect.GeneratedMethodAccessor41.invoke(Unknown Source)
    java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    java.base/java.lang.reflect.Method.invoke(Method.java:568)
    rpc.client.ClientPool.invoke0(ClientPool.java:133)
    rpc.client.ClientPool$1.invoke(ClientPool.java:117)
    jdk.proxy2/jdk.proxy2.$Proxy85.battleMsgNotifyResponse(Unknown Source)
    jdk.internal.reflect.GeneratedMethodAccessor41.invoke(Unknown Source)
    java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    java.base/java.lang.reflect.Method.invoke(Method.java:568)
    common.rpc.client.proxy.DefaultRpcProxyFactory$RpcClientProxy.invoke(DefaultRpcProxyFactory.java:80)
    jdk.proxy2/jdk.proxy2.$Proxy85.battleMsgNotifyResponse(Unknown Source)
    battle.service.notify.BattleSubWriter.battleMsgNotifyResponse(BattleSubWriter.java:139)
    battle.service.notify.BattleSubWriter.handleBattleServerResponse(BattleSubWriter.java:110)
    battle.service.notify.BattleSubWriter.doWork(BattleSubWriter.java:67)
    java.base/java.lang.Thread.run(Thread.java:842)

@whyuan
Copy link
Author

whyuan commented Mar 12, 2024

I found that in methods like com.linecorp.armeria.client.limit.AbstractConcurrencyLimitingClient#execute(), the req object is processed asynchronously. The corresponding exception handling is also in the asynchronous callback, passed through CompletableFuture#completeExceptionally(). It seems that these exceptions do not handle ByteBuf#release()? I tried adding httpReq.abort() to the exception handling of httpResponse's handle callback in THttpClientDelegate as shown below, and there were no more warnings about buf memory leaks. Is this modification correct?
image

@jrhee17
Copy link
Contributor

jrhee17 commented Mar 12, 2024

Thanks for following up on this, I think your analysis is right 👍

We're allocating a pooled buffer for thrift requests, so we need to make sure we release it correctly (or it is a leak) by aborting the request.

armeria/thrift/thrift0.13/src/main/java/com/linecorp/armeria/internal/client/thrift/THttpClientDelegate.java

Line 134 in 75d5af1

final ByteBuf buf = ctx.alloc().buffer(128);

Is this modification correct?

Since AbstractConcurrencyLimitingClient isn't the only place where requests can fail and thrift requests are by nature unary, I agree the handling point should be from thrift related clients.

I agree with your proposal, although I think just handling inside handlePreDecodeException may be better to handle more cases. Were you interested in sending in a PR? Otherwise, we'll fix it and include the fix in the next release

@jrhee17 jrhee17 added this to the 1.28.0 milestone Mar 12, 2024
@jrhee17 jrhee17 added defect and removed needs info labels Mar 12, 2024
@whyuan
Copy link
Author

whyuan commented Mar 12, 2024

Thanks for fixing it in the next version.🤝🤝

@jrhee17
Copy link
Contributor

jrhee17 commented Mar 12, 2024

Thanks! We'll handle it then 🙇 🙇 🙇

@jrhee17 jrhee17 mentioned this issue Mar 27, 2024
Merged
jrhee17 added a commit that referenced this issue Apr 3, 2024
@jrhee17
Handle a leak when thrift requests are aborted early (#5535)
3fc6f8f 
Motivation:

We received a report where thrift was leaking pooled byte buffers.
We use pooled byte buffers to create the thrift request in
`THttpClientDelegate` assuming
that it will be released when sent on the wire for normal cases.


https://github.com/line/armeria/blob/7f302505a05d24e7704bcffc43fab6337344cb2c/thrift/thrift0.13/src/main/java/com/linecorp/armeria/internal/client/thrift/THttpClientDelegate.java#L134

However, if any requests don't have a chance to reach the wire, they
will leak the byte buffer.
I believe it is reasonable to assume that requests not decoded properly
should be cleaned up. Hence, I propose we do an extra `req.abort` inside
`handlePreDecodeException`.

Modifications:

- Modify `handlePreDecodeException` to accept an additional
`HttpRequest` parameter
- Abort the request with the exception if exists

Result:

- #5481

<!--
Visit this URL to learn more about how to write a pull request
description:

https://armeria.dev/community/developer-guide#how-to-write-pull-request-description
-->
@jrhee17
Copy link
Contributor

jrhee17 commented Apr 3, 2024

Handled by #5535

@jrhee17 jrhee17 closed this as completed Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
defect
Projects
None yet
Milestone
1.28.0
Development

No branches or pull requests
2 participants
@whyuan @jrhee17