[#2060] fix(server): Fix memory leaks when reaching memory limit #2058

lwllvyb · 2024-08-19T07:37:33Z

What changes were proposed in this pull request?

Fix shuffle server memory leaks when reaching memory limit.

Why are the changes needed?

When enabling Netty, on the shuffle server side, Netty will allocate memory for SEND_SHUFFLE_DATA_REQUEST request. However, when the memory limit is reached, an OutOfDirectMemoryError will be thrown and the decoding process for this message will fail. This will cause the bytebuf allocated successfully in the previous batch in this message to not be released, resulting in memory leaks.

Fix: #2060

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing UTs

lwllvyb · 2024-08-19T07:49:13Z

The error stack:

[2024-08-08 20:49:31.754] [epollEventLoopGroup-3-2] [ERROR] ShuffleServerNettyHandler - exception caught /*:38381
io.netty.util.internal.OutOfDirectMemoryError: failed to allocate 65536 byte(s) of direct memory (used: 17179815063, max: 17179869184)
at io.netty.util.internal.PlatformDependent.incrementMemoryCounter(PlatformDependent.java:843)
at io.netty.util.internal.PlatformDependent.allocateDirectNoCleaner(PlatformDependent.java:772)
at io.netty.buffer.UnpooledUnsafeNoCleanerDirectByteBuf.allocateDirect(UnpooledUnsafeNoCleanerDirectByteBuf.java:30)
at io.netty.buffer.UnpooledByteBufAllocator$InstrumentedUnpooledUnsafeNoCleanerDirectByteBuf.allocateDirect(UnpooledByteBufAllocator.java:186)
at io.netty.buffer.UnpooledDirectByteBuf.(UnpooledDirectByteBuf.java:64) at io.netty.buffer.UnpooledUnsafeDirectByteBuf.(UnpooledUnsafeDirectByteBuf.java:41)
at io.netty.buffer.UnpooledUnsafeNoCleanerDirectByteBuf.(UnpooledUnsafeNoCleanerDirectByteBuf.java:25)
at io.netty.buffer.UnpooledByteBufAllocator$InstrumentedUnpooledUnsafeNoCleanerDirectByteBuf.(UnpooledByteBufAllocator.java:181)
at io.netty.buffer.UnpooledByteBufAllocator.newDirectBuffer(UnpooledByteBufAllocator.java:91)
at io.netty.buffer.AbstractByteBufAllocator.directBuffer(AbstractByteBufAllocator.java:188)
at io.netty.buffer.AbstractByteBufAllocator.directBuffer(AbstractByteBufAllocator.java:179)
at io.netty.channel.unix.PreferredDirectByteBufAllocator.ioBuffer(PreferredDirectByteBufAllocator.java:53)
at io.netty.channel.DefaultMaxMessagesRecvByteBufAllocator$MaxMessageHandle.allocate(DefaultMaxMessagesRecvByteBufAllocator.java:120)
at io.netty.channel.epoll.EpollRecvByteAllocatorHandle.allocate(EpollRecvByteAllocatorHandle.java:75)
at io.netty.channel.epoll.AbstractEpollStreamChannel$EpollStreamUnsafe.epollInReady(AbstractEpollStreamChannel.java:786)
at io.netty.channel.epoll.EpollEventLoop.processReady(EpollEventLoop.java:501)
at io.netty.channel.epoll.EpollEventLoop.run(EpollEventLoop.java:399)
at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997)
at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
at java.lang.Thread.run(Thread.java:750)

github-actions · 2024-08-19T08:29:27Z

Test Results

2 926 files +31 2 926 suites +31 5h 57m 50s ⏱️ + 10m 45s
1 043 tests ± 0 1 041 ✅ + 1 2 💤 ±0 0 ❌ - 1
13 033 runs +60 13 003 ✅ +61 30 💤 ±0 0 ❌ - 1

Results for commit 6903a21. ± Comparison against base commit f9b4c0e.

♻️ This comment has been updated with latest results.

rickyma · 2024-08-19T12:39:25Z

I think we need to first avoid the occurrence of OOM. The occurrence of OOM is either due to unreasonable memory configurations or bugs in the code. As for how the code should handle after OOM, I don't think it's very important, because the server has already malfunctioned at this point. Even if there is a memory leak, it's actually not important anymore.

lwllvyb · 2024-08-28T06:43:42Z

I think we need to first avoid the occurrence of OOM. The occurrence of OOM is either due to unreasonable memory configurations or bugs in the code. As for how the code should handle after OOM, I don't think it's very important, because the server has already malfunctioned at this point. Even if there is a memory leak, it's actually not important anymore.

So can this PR be accepted ? Or should I just cancel this PR? @rickyma

rickyma · 2024-08-28T07:52:20Z

I'm OK with this PR. But it's meaningless. When an OOM error occurs, this PR will not help much.

lwllvyb · 2024-08-28T08:27:57Z

I'm OK with this PR. But it's meaningless. When an OOM error occurs, this PR will not help much.

This PR is not to prevent the OOM exception, but to ensure that the pre-allocated ByteBuf can be released normally.

jerqi · 2024-08-28T09:30:18Z

I'm OK with this PR. But it's meaningless. When an OOM error occurs, this PR will not help much.

This PR is not to prevent the OOM exception, but to ensure that the pre-allocated ByteBuf can be released normally.

You shouldn't catch OOM exception. If it throws OOM, more errors may throw. You can't recover it by just catching it.

lwllvyb · 2024-08-28T12:01:52Z

I'm OK with this PR. But it's meaningless. When an OOM error occurs, this PR will not help much.

This PR is not to prevent the OOM exception, but to ensure that the pre-allocated ByteBuf can be released normally.

You shouldn't catch OOM exception. If it throws OOM, more errors may throw. You can't recover it by just catching it.

I don't understand what you mean.
If the OOM exception happen, how we deal with the pre-allocated ByteBuf ? Reconfigure and restart the server , or other ways?

jerqi · 2024-08-28T12:19:15Z

I'm OK with this PR. But it's meaningless. When an OOM error occurs, this PR will not help much.

This PR is not to prevent the OOM exception, but to ensure that the pre-allocated ByteBuf can be released normally.

You shouldn't catch OOM exception. If it throws OOM, more errors may throw. You can't recover it by just catching it.

I don't understand what you mean. If the OOM exception happen, how we deal with the pre-allocated ByteBuf ? Reconfigure and restart the server , or other ways?

If it will OOM, the java process should exit.

lwllvyb · 2024-08-30T10:37:15Z

I'm OK with this PR. But it's meaningless. When an OOM error occurs, this PR will not help much.

This PR is not to prevent the OOM exception, but to ensure that the pre-allocated ByteBuf can be released normally.

You shouldn't catch OOM exception. If it throws OOM, more errors may throw. You can't recover it by just catching it.

I don't understand what you mean. If the OOM exception happen, how we deal with the pre-allocated ByteBuf ? Reconfigure and restart the server , or other ways?

If it will OOM, the java process should exit.

I need to clarify that it is not OOM but OutOfDirectMemoryError. From the stack trace, we can see that the server did not exit.

rickyma · 2024-08-30T11:38:30Z

Yeah, it is a Netty's internal OOM error. It's meaningless to catch this exception. On the other hand, this PR is harmless.

So I choose to remain neutral.

zhengchenyu · 2024-09-24T06:24:01Z

I have also encountered such a problem and fixed it with a similar method. Although I also think that we can solve this problem by increasing MaxDirectMemorySize or reducing rss.server.buffer.capacity. But it is difficult for many people to find this reasonable value. If at a certain peak usage moment, direct memory happens to exceed MaxDirectMemorySize, leaks will occur. I think it is necessary to merge this pr.

zhengchenyu · 2024-09-24T06:26:34Z

I'm OK with this PR. But it's meaningless. When an OOM error occurs, this PR will not help much.

This PR is not to prevent the OOM exception, but to ensure that the pre-allocated ByteBuf can be released normally.

You shouldn't catch OOM exception. If it throws OOM, more errors may throw. You can't recover it by just catching it.

I don't understand what you mean. If the OOM exception happen, how we deal with the pre-allocated ByteBuf ? Reconfigure and restart the server , or other ways?

If it will OOM, the java process should exit.

This OOM is not java OOM. The OOM is that the direct memory used exceeds the direct memory limit of Netty. If Netty's direct memory limit is too low, it will easily trigger io.netty.util.internal.OutOfDirectMemoryError without causing java process exit.

jerqi · 2024-09-24T06:32:25Z

I'm OK with this PR. But it's meaningless. When an OOM error occurs, this PR will not help much.

This PR is not to prevent the OOM exception, but to ensure that the pre-allocated ByteBuf can be released normally.

You shouldn't catch OOM exception. If it throws OOM, more errors may throw. You can't recover it by just catching it.

I don't understand what you mean. If the OOM exception happen, how we deal with the pre-allocated ByteBuf ? Reconfigure and restart the server , or other ways?

If it will OOM, the java process should exit.

This OOM is not java OOM. The OOM is that the direct memory used exceeds the direct memory limit of Netty. If Netty's direct memory limit is too low, it will easily trigger io.netty.util.internal.OutOfDirectMemoryError without causing java process exit.

Is io.netty.util.internal.OutOfDirectMemoryError a subclass of OutOfMemoryError? It's a little weird to catch an error. Because we can't recover an error usually.

zhengchenyu · 2024-09-24T06:39:52Z

I'm OK with this PR. But it's meaningless. When an OOM error occurs, this PR will not help much.

This PR is not to prevent the OOM exception, but to ensure that the pre-allocated ByteBuf can be released normally.

You shouldn't catch OOM exception. If it throws OOM, more errors may throw. You can't recover it by just catching it.

I don't understand what you mean. If the OOM exception happen, how we deal with the pre-allocated ByteBuf ? Reconfigure and restart the server , or other ways?

If it will OOM, the java process should exit.

This OOM is not java OOM. The OOM is that the direct memory used exceeds the direct memory limit of Netty. If Netty's direct memory limit is too low, it will easily trigger io.netty.util.internal.OutOfDirectMemoryError without causing java process exit.

Is io.netty.util.internal.OutOfDirectMemoryError a subclass of OutOfMemoryError? It's a little weird to catch an error. Because we can't recover an error usually.

io.netty.util.internal.OutOfDirectMemoryError is not a subclass of OutOfMemoryError. Even though it's a little weird to catch an error, but this error can not make the process exit.

jerqi · 2024-09-24T06:41:20Z

o.netty.util.internal.OutOfDirectMemoryError

I read the code

package io.netty.util.internal;

import java.nio.ByteBuffer;

/**
 * {@link OutOfMemoryError} that is throws if {@link PlatformDependent#allocateDirectNoCleaner(int)} can not allocate
 * a new {@link ByteBuffer} due memory restrictions.
 */
public final class OutOfDirectMemoryError extends OutOfMemoryError {
    private static final long serialVersionUID = 4228264016184011555L;

    OutOfDirectMemoryError(String s) {
        super(s);
    }
}

jerqi · 2024-09-24T06:42:49Z

Maybe we should add some comments to explain this.

zhengchenyu · 2024-09-24T06:50:08Z

Sorry, I write it wrongly. Below is the netty source code. If we set DIRECT_MEMORY_LIMIT to not a big number, will throw OutOfDirectMemoryError. But the jvm process will not exit, but the ByteBuf will be leaked.

    private static void incrementMemoryCounter(int capacity) {
        if (DIRECT_MEMORY_COUNTER != null) {
            long newUsedMemory = DIRECT_MEMORY_COUNTER.addAndGet(capacity);
            if (newUsedMemory > DIRECT_MEMORY_LIMIT) {
                DIRECT_MEMORY_COUNTER.addAndGet(-capacity);
                throw new OutOfDirectMemoryError("failed to allocate " + capacity
                        + " byte(s) of direct memory (used: " + (newUsedMemory - capacity)
                        + ", max: " + DIRECT_MEMORY_LIMIT + ')');
            }
        }
    }

jerqi · 2024-09-24T06:58:28Z

Sorry, I write it wrongly. Below is the netty source code. If we set DIRECT_MEMORY_LIMIT to not a big number, will throw OutOfDirectMemoryError. But the jvm process will not exit, but the ByteBuf will be leaked.

    private static void incrementMemoryCounter(int capacity) {
        if (DIRECT_MEMORY_COUNTER != null) {
            long newUsedMemory = DIRECT_MEMORY_COUNTER.addAndGet(capacity);
            if (newUsedMemory > DIRECT_MEMORY_LIMIT) {
                DIRECT_MEMORY_COUNTER.addAndGet(-capacity);
                throw new OutOfDirectMemoryError("failed to allocate " + capacity
                        + " byte(s) of direct memory (used: " + (newUsedMemory - capacity)
                        + ", max: " + DIRECT_MEMORY_LIMIT + ')');
            }
        }
    }

Maybe it's ok to add comments to merge this pull request.

zhengchenyu · 2024-09-24T07:10:30Z

common/src/main/java/org/apache/uniffle/common/netty/protocol/SendShuffleDataRequest.java

+ } catch (Throwable t) {
+ if (!shuffleBlockInfoList.isEmpty()) {
+ shuffleBlockInfoList.forEach(sbi -> sbi.getData().release());
+ }


Do we need check empty?

The check condition is redundant. I will remove the check.

rickyma · 2024-09-24T07:56:03Z

I have also encountered such a problem and fixed it with a similar method. Although I also think that we can solve this problem by increasing MaxDirectMemorySize or reducing rss.server.buffer.capacity. But it is difficult for many people to find this reasonable value. If at a certain peak usage moment, direct memory happens to exceed MaxDirectMemorySize, leaks will occur. I think it is necessary to merge this pr.

If the users follow this guidance, this error should not happen, or else there exists a bug.

lwllvyb · 2024-10-15T02:24:01Z

Ping @jerqi @rickyma @zhengchenyu
Could you please take some time to review this PR? If it doesn't need to be merged, please let me know. This PR has been verified internally.

rickyma

This PR is harmless.

I'm fine with this. Someone else can take a look.

jerqi · 2024-10-15T09:32:19Z

Could you add some comments for this code?

lwllvyb · 2024-10-16T03:23:41Z

Could you add some comments for this code?

Thanks for your review. I added some comments to the code. Please take the time to review again when it is convenient for you.
Thanks again. @jerqi

jerqi

LGTM.

rickyma · 2024-10-16T07:35:35Z

common/src/main/java/org/apache/uniffle/common/netty/protocol/SendShuffleDataRequest.java

+ } catch (Throwable t) {
+ // An OutOfDirectMemoryError will be thrown, when the direct memory reaches the limit.
+ // OutOfDirectMemoryError does not cause the jvm exit and cause the direct memory leak.
+ // Warning: Please set DIRECT_MEMORY_LIMIT to a reasonable value.


DIRECT_MEMORY_LIMIT does not exist? Maybe DIRECT_MEMORY_LIMIT -> MAX_DIRECT_MEMORY_SIZE?

You can also put the link of the guidance in the comment.

rickyma · 2024-10-16T07:37:37Z

common/src/main/java/org/apache/uniffle/common/netty/protocol/SendShuffleDataRequest.java

+ shuffleBlockInfoList.add(Decoders.decodeShuffleBlockInfo(byteBuf));
+ } catch (Throwable t) {
+ // An OutOfDirectMemoryError will be thrown, when the direct memory reaches the limit.
+ // OutOfDirectMemoryError does not cause the jvm exit and cause the direct memory leak.


OutOfDirectMemoryError does not cause the JVM to exit and it may lead to direct memory leaks.

Fixed.
@rickyma

codecov-commenter · 2024-10-19T06:45:52Z

Codecov Report

Attention: Patch coverage is 25.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 52.86%. Comparing base (5ddcc28) to head (6903a21).
Report is 125 commits behind head on master.

Files with missing lines	Patch %	Lines
.../common/netty/protocol/SendShuffleDataRequest.java	25.00%	6 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2058      +/-   ##
============================================
+ Coverage     52.77%   52.86%   +0.09%     
- Complexity     2498     3422     +924     
============================================
  Files           398      518     +120     
  Lines         18135    28359   +10224     
  Branches       1660     2650     +990     
============================================
+ Hits           9570    14991    +5421     
- Misses         7981    12400    +4419     
- Partials        584      968     +384

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rickyma · 2024-10-19T08:40:30Z

common/src/main/java/org/apache/uniffle/common/netty/protocol/SendShuffleDataRequest.java

+ // An OutOfDirectMemoryError will be thrown, when the direct memory reaches the limit.
+ // OutOfDirectMemoryError will not cause the JVM to exit, but may lead to direct memory
+ // leaks.
+ // Warning: You can refer to docs/server_guide.md to set MAX_DIRECT_MEMORY_SIZE to a


It's not a warning.
Maybe: Warning: -> Note:

Fixed it as you suggested.

lwllvyb changed the title ~~fix(server): Fix memory leak when reach memory limit~~ [#2060]fix(server): Fix memory leak when reach memory limit Aug 19, 2024

jerqi requested a review from rickyma August 19, 2024 10:47

zhengchenyu reviewed Sep 24, 2024

View reviewed changes

lwllvyb force-pushed the memoryLeak branch from 03ab441 to 9769bea Compare October 10, 2024 08:24

rickyma previously approved these changes Oct 15, 2024

View reviewed changes

lwllvyb dismissed rickyma’s stale review via 3198146 October 16, 2024 03:19

lwllvyb force-pushed the memoryLeak branch from 9769bea to 3198146 Compare October 16, 2024 03:19

jerqi previously approved these changes Oct 16, 2024

View reviewed changes

rickyma reviewed Oct 16, 2024

View reviewed changes

rickyma changed the title ~~[#2060]fix(server): Fix memory leak when reach memory limit~~ [#2060] fix(server): Fix memory leaks when reaching memory limit Oct 16, 2024

lwllvyb dismissed jerqi’s stale review via 6903a21 October 18, 2024 04:09

lwllvyb force-pushed the memoryLeak branch from 3198146 to 6903a21 Compare October 18, 2024 04:09

lwllvyb closed this Oct 19, 2024

lwllvyb reopened this Oct 19, 2024

lwllvyb closed this Oct 19, 2024

lwllvyb reopened this Oct 19, 2024

rickyma reviewed Oct 19, 2024

View reviewed changes

fix(server): Fix memory leak when reach memory limit

e82b230

lwllvyb force-pushed the memoryLeak branch from 6903a21 to e82b230 Compare October 21, 2024 07:56

rickyma approved these changes Oct 21, 2024

View reviewed changes

rickyma merged commit c3dd749 into apache:master Oct 21, 2024
41 of 42 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#2060] fix(server): Fix memory leaks when reaching memory limit #2058

[#2060] fix(server): Fix memory leaks when reaching memory limit #2058

lwllvyb commented Aug 19, 2024 •

edited by rickyma

Loading

lwllvyb commented Aug 19, 2024

github-actions bot commented Aug 19, 2024 •

edited

Loading

rickyma commented Aug 19, 2024

lwllvyb commented Aug 28, 2024

rickyma commented Aug 28, 2024

lwllvyb commented Aug 28, 2024

jerqi commented Aug 28, 2024

lwllvyb commented Aug 28, 2024

jerqi commented Aug 28, 2024

lwllvyb commented Aug 30, 2024

rickyma commented Aug 30, 2024

zhengchenyu commented Sep 24, 2024

zhengchenyu commented Sep 24, 2024

jerqi commented Sep 24, 2024

zhengchenyu commented Sep 24, 2024

jerqi commented Sep 24, 2024

jerqi commented Sep 24, 2024

zhengchenyu commented Sep 24, 2024

jerqi commented Sep 24, 2024

zhengchenyu Sep 24, 2024

lwllvyb Oct 10, 2024

rickyma commented Sep 24, 2024 •

edited

Loading

lwllvyb commented Oct 15, 2024

rickyma left a comment

jerqi commented Oct 15, 2024

lwllvyb commented Oct 16, 2024

jerqi left a comment

rickyma Oct 16, 2024

rickyma Oct 16, 2024

rickyma Oct 16, 2024

lwllvyb Oct 18, 2024

codecov-commenter commented Oct 19, 2024 •

edited

Loading

rickyma Oct 19, 2024

lwllvyb Oct 21, 2024

[#2060] fix(server): Fix memory leaks when reaching memory limit #2058

[#2060] fix(server): Fix memory leaks when reaching memory limit #2058

Conversation

lwllvyb commented Aug 19, 2024 • edited by rickyma Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

lwllvyb commented Aug 19, 2024

github-actions bot commented Aug 19, 2024 • edited Loading

Test Results

rickyma commented Aug 19, 2024

lwllvyb commented Aug 28, 2024

rickyma commented Aug 28, 2024

lwllvyb commented Aug 28, 2024

jerqi commented Aug 28, 2024

lwllvyb commented Aug 28, 2024

jerqi commented Aug 28, 2024

lwllvyb commented Aug 30, 2024

rickyma commented Aug 30, 2024

zhengchenyu commented Sep 24, 2024

zhengchenyu commented Sep 24, 2024

jerqi commented Sep 24, 2024

zhengchenyu commented Sep 24, 2024

jerqi commented Sep 24, 2024

jerqi commented Sep 24, 2024

zhengchenyu commented Sep 24, 2024

jerqi commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickyma commented Sep 24, 2024 • edited Loading

lwllvyb commented Oct 15, 2024

rickyma left a comment

Choose a reason for hiding this comment

jerqi commented Oct 15, 2024

lwllvyb commented Oct 16, 2024

jerqi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 19, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lwllvyb commented Aug 19, 2024 •

edited by rickyma

Loading

github-actions bot commented Aug 19, 2024 •

edited

Loading

rickyma commented Sep 24, 2024 •

edited

Loading

codecov-commenter commented Oct 19, 2024 •

edited

Loading