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

WriteBufferWaterMark's TOTAL_PENDING_SIZE_UPDATER leak when limit the io.netty.eventexecutor.maxPendingTasks(taskQueue) size. #8343

Closed
caorong opened this issue Oct 5, 2018 · 3 comments
Assignees
Labels
Milestone

Comments

@caorong
Copy link

caorong commented Oct 5, 2018

Expected behavior

In production env, we meet huge small request need to writeAndFlush, in order to avoid oom, reduce large memory to old area, and limit the network flow.

I both set System.setProperty("io.netty.eventLoop.maxPendingTasks", "2048"); and

bootstrap.option(ChannelOption.WRITE_BUFFER_WATER_MARK, new WriteBufferWaterMark(8 * 1024 , 32 * 1024));

but I found that, someTime, when many request comes in a short time, channel.isWriteable() is always false, but after that even if no request, it it still Talse.

It should be True, but actually sometime it is False and never comeback.

Actual behavior

SomeTime channel.isWriteable() is False, and even when no more WriteAndFlushTask, channel.isWriteable() is still False.

Steps to reproduce

I review the source code.

when I call WriteAndFlush, netty will run in following steps.

  1. new a WriteAndFlushTask, It will incrementPendingOutboundBytes.
  2. if the task executed in eventLoop, It will decrementPendingOutboundBytes.

so, if all task are created and run, no problem happends.

but if I limit the taskQueue and when request count large than its capacity, some WriteAndFlushTask maybe init, but rejected by eventloop.

at this time, WriteBufferWaterMark's TOTAL_PENDING_SIZE_UPDATER leak happends.

my solution

only use writeBufferWatermark and check isWriteable or only limit the taskQueue but abandon check channel.isWriteable().

how about add a common check, if rejected Task is WriteTask / WriteAndFlushTask, will decrementPendingOutboundBytes avoid leak.

my now solution is new NioEventLoopGroup with custom RejectedExecutionHandler

Is my usage wrong, or if more grace way to handle this problem?

Netty version

4.1.29.Final

JVM version (e.g. java -version)

java8

OS version (e.g. uname -a)

linux_x86_64

@normanmaurer
Copy link
Member

@caorong good catch... let me fix it.

@normanmaurer
Copy link
Member

@caorong PTAL #8349

@normanmaurer normanmaurer self-assigned this Oct 11, 2018
@normanmaurer normanmaurer added this to the 4.1.31.Final milestone Oct 11, 2018
normanmaurer added a commit that referenced this issue Oct 11, 2018
…ails.

Motivation:

Currently we may end up in the situation that we incremented the pending bytes before submitting the AbstractWriteTask but never decrement these again if the submitting of the task fails. This may result in incorrect watermark handling.

Modifications:

- Correctly decrement pending bytes if subimitting of task fails and also ensure we recycle it correctly.
- Add unit test.

Result:

Fixes #8343.
@caorong
Copy link
Author

caorong commented Oct 11, 2018

thanks to fix this!

normanmaurer added a commit that referenced this issue Oct 11, 2018
…ails. (#8349)

Motivation:

Currently we may end up in the situation that we incremented the pending bytes before submitting the AbstractWriteTask but never decrement these again if the submitting of the task fails. This may result in incorrect watermark handling.

Modifications:

- Correctly decrement pending bytes if subimitting of task fails and also ensure we recycle it correctly.
- Add unit test.

Result:

Fixes #8343.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants