You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2022/02/24 04:33:15 UTC
[GitHub] [thrift] ferrirW opened a new pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
ferrirW opened a new pull request #2533:
URL: https://github.com/apache/thrift/pull/2533
<!-- Explain the changes in the pull request below: -->
We encountered a problem that caused the CPU to fill up (24 thread, 40 core), it seems to be due to the readBufferBytesAllocated not being properly calculated and the frameSize is large.
So i think that even if the variable is correctly evaluated, it is necessary to add a separate parameter limit to the frameSize, rather than reusing the maxReadBufferBytes.
ps. this large frame should be come from security check detection, also it could be attack
24 core is full
big request caused inifinite loop
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [ x ] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? (not required for trivial changes)
- [ x ] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [ ] Did you squash your changes to a single commit? (not required, but preferred)
- [ ] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [ ] If your change does not involve any code, include `[skip ci]` anywhere in the commit message to free up build resources.
<!--
The Contributing Guide at:
https://github.com/apache/thrift/blob/master/CONTRIBUTING.md
has more details and tips for committing properly.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ferrirW commented on a change in pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
ferrirW commented on a change in pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#discussion_r815548094
##########
File path: lib/java/src/org/apache/thrift/server/AbstractNonblockingServer.java
##########
@@ -51,6 +51,7 @@
public static abstract class AbstractNonblockingServerArgs<T extends AbstractNonblockingServerArgs<T>> extends AbstractServerArgs<T> {
public long maxReadBufferBytes = 256 * 1024 * 1024;
+ public long maxReadFrameBytes = 256 * 1024 * 1024;
Review comment:
And if you are asking why need this constant:
I want to separate this place, so method handleRead() could cleanupSelectionKey when the frame size is large but does not reach the MAX_READ_BUFFER_BYTES limit. Then i can make the big request failed quickly, so the select method will not cycle through the CPU indefinitely as shown above.
```
if (frameSize > MAX_READ_FRAME_BYTES) {
LOGGER.error("Read a frame size of " + frameSize
+ ", which is bigger than the maximum allowable frame size "
+ MAX_READ_FRAME_BYTES + " for ALL connections.");
return false;
}
// if this frame will push us over the memory limit, then return.
// with luck, more memory will free up the next time around.
if (readBufferBytesAllocated.get() + frameSize > MAX_READ_BUFFER_BYTES) {
return true;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ferrirW commented on pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
ferrirW commented on pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#issuecomment-1053880981
> Is it a good idea to use TConfiguration maxFrameSize？
1. static final constant DEFAULT_MAX_FRAME_SIZE is not able for customization.
2. so i have to use trans_.getConfiguration().getMaxFrameSize(), but that means many class need to be modified(TNonblockingSocket, TNonblockingServerSocket, AbstractServerTransportArgs and NonblockingAbstractServerSocketArgs)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on a change in pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
Jens-G commented on a change in pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#discussion_r815304655
##########
File path: lib/java/src/org/apache/thrift/server/AbstractNonblockingServer.java
##########
@@ -64,6 +65,7 @@ public AbstractNonblockingServerArgs(TNonblockingServerTransport transport) {
* right into an out of memory exception, rather than waiting.
*/
final long MAX_READ_BUFFER_BYTES;
+ final long MAX_READ_FRAME_BYTES;
Review comment:
Why do we need another constant?
##########
File path: lib/java/src/org/apache/thrift/server/AbstractNonblockingServer.java
##########
@@ -51,6 +51,7 @@
public static abstract class AbstractNonblockingServerArgs<T extends AbstractNonblockingServerArgs<T>> extends AbstractServerArgs<T> {
public long maxReadBufferBytes = 256 * 1024 * 1024;
+ public long maxReadFrameBytes = 256 * 1024 * 1024;
Review comment:
Why do we need another constant?
##########
File path: lib/java/src/org/apache/thrift/server/AbstractNonblockingServer.java
##########
@@ -352,9 +355,10 @@ public boolean read() {
// if this frame will always be too large for this server, log the
// error and close the connection.
- if (frameSize > MAX_READ_BUFFER_BYTES) {
+ if (frameSize > MAX_READ_FRAME_BYTES) {
Review comment:
Why do we need another constant?
##########
File path: lib/java/src/org/apache/thrift/server/AbstractNonblockingServer.java
##########
@@ -73,6 +75,7 @@ public AbstractNonblockingServerArgs(TNonblockingServerTransport transport) {
public AbstractNonblockingServer(AbstractNonblockingServerArgs args) {
super(args);
MAX_READ_BUFFER_BYTES = args.maxReadBufferBytes;
+ MAX_READ_FRAME_BYTES = args.maxReadFrameBytes;
Review comment:
Why do we need another constant?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ferrirW commented on a change in pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
ferrirW commented on a change in pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#discussion_r815548094
##########
File path: lib/java/src/org/apache/thrift/server/AbstractNonblockingServer.java
##########
@@ -51,6 +51,7 @@
public static abstract class AbstractNonblockingServerArgs<T extends AbstractNonblockingServerArgs<T>> extends AbstractServerArgs<T> {
public long maxReadBufferBytes = 256 * 1024 * 1024;
+ public long maxReadFrameBytes = 256 * 1024 * 1024;
Review comment:
And if you are asking why need this constant:
I want to separate this two place, so method handleRead() could cleanupSelectionKey when the frame size is large but does not reach the MAX_READ_BUFFER_BYTES limit. Then the select method will not cycle through the CPU indefinitely as shown above.
```
if (frameSize > MAX_READ_FRAME_BYTES) {
LOGGER.error("Read a frame size of " + frameSize
+ ", which is bigger than the maximum allowable frame size "
+ MAX_READ_FRAME_BYTES + " for ALL connections.");
return false;
}
// if this frame will push us over the memory limit, then return.
// with luck, more memory will free up the next time around.
if (readBufferBytesAllocated.get() + frameSize > MAX_READ_BUFFER_BYTES) {
return true;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ferrirW commented on pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
ferrirW commented on pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#issuecomment-1058880825
> Using 0.9.1 is discouraged anyways
Our project have many clients exist in projects that other people are responsible for. So each upgrade takes a very long time. Most likely it won't upgrade for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
Jens-G commented on pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#issuecomment-1053985014
+1 LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ferrirW commented on pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
ferrirW commented on pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#issuecomment-1053882220
> For the sake of consistency, we should aim to use
> DEFAULT_RECURSION_LIMIT = 64;
> DEFAULT_MAX_MESSAGE_SIZE = 100 * 1024 * 1024; // 100 MB
> DEFAULT_MAX_FRAME_SIZE = 16384000; // this value is used consistently across all Thrift libraries
Is it a good idea to use TConfiguration maxFrameSize？
1. static final constant DEFAULT_MAX_FRAME_SIZE is not able for customization.
2. so i have to use trans_.getConfiguration().getMaxFrameSize(), but that means many class need to be modified(TNonblockingSocket, TNonblockingServerSocket, AbstractServerTransportArgs and NonblockingAbstractServerSocketArgs)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ferrirW closed pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
ferrirW closed pull request #2533:
URL: https://github.com/apache/thrift/pull/2533
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ferrirW removed a comment on pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
ferrirW removed a comment on pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#issuecomment-1053880981
> Is it a good idea to use TConfiguration maxFrameSize？
1. static final constant DEFAULT_MAX_FRAME_SIZE is not able for customization.
2. so i have to use trans_.getConfiguration().getMaxFrameSize(), but that means many class need to be modified(TNonblockingSocket, TNonblockingServerSocket, AbstractServerTransportArgs and NonblockingAbstractServerSocketArgs)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ferrirW commented on pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
ferrirW commented on pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#issuecomment-1053881155
> For the sake of consistency, we should aim to use
> DEFAULT_RECURSION_LIMIT = 64;
> DEFAULT_MAX_MESSAGE_SIZE = 100 * 1024 * 1024; // 100 MB
> DEFAULT_MAX_FRAME_SIZE = 16384000; // this value is used consistently across all Thrift libraries
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ferrirW commented on a change in pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
ferrirW commented on a change in pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#discussion_r815543315
##########
File path: lib/java/src/org/apache/thrift/server/AbstractNonblockingServer.java
##########
@@ -51,6 +51,7 @@
public static abstract class AbstractNonblockingServerArgs<T extends AbstractNonblockingServerArgs<T>> extends AbstractServerArgs<T> {
public long maxReadBufferBytes = 256 * 1024 * 1024;
+ public long maxReadFrameBytes = 256 * 1024 * 1024;
Review comment:
Just because DEFAULT_MAX_FRAME_SIZE is not exist in 0.9.2.
I will fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ferrirW edited a comment on pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
ferrirW edited a comment on pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#issuecomment-1058875922
> Starting a Gradle Daemon (subsequent builds will be faster)
This failed because AbstractServerTransportArgs.maxFrameSize is not init. And this problem has been solved.
But there seems to be some checks that cannot be pass. Could you please help to check it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ferrirW removed a comment on pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
ferrirW removed a comment on pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#issuecomment-1053881155
> For the sake of consistency, we should aim to use
> DEFAULT_RECURSION_LIMIT = 64;
> DEFAULT_MAX_MESSAGE_SIZE = 100 * 1024 * 1024; // 100 MB
> DEFAULT_MAX_FRAME_SIZE = 16384000; // this value is used consistently across all Thrift libraries
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ferrirW commented on pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
ferrirW commented on pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#issuecomment-1060145988
Actually, I think this problem also exist in the latest version. But I don't have much time to reproduce. So If anyone else is experiencing this problem(CPU is full and consumed by select()), they could try use this PR to fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ferrirW edited a comment on pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
ferrirW edited a comment on pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#issuecomment-1060145988
Actually, I think this problem also exist in the latest version. But I don't have much time to reproduce. So If anyone else is experiencing this problem, they can try use this PR (and limit frame size to a reasonable value) to fix it.
The problem phenomenon:
1. CPU is full
2. Most time consumed on SelectorThread.select()
3. FrameBuffer.read() ends quickly at "if (frameSize > MAX_READ_BUFFER_BYTES))"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ferrirW commented on a change in pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
ferrirW commented on a change in pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#discussion_r815548094
##########
File path: lib/java/src/org/apache/thrift/server/AbstractNonblockingServer.java
##########
@@ -51,6 +51,7 @@
public static abstract class AbstractNonblockingServerArgs<T extends AbstractNonblockingServerArgs<T>> extends AbstractServerArgs<T> {
public long maxReadBufferBytes = 256 * 1024 * 1024;
+ public long maxReadFrameBytes = 256 * 1024 * 1024;
Review comment:
And if you are asking why need this constant:
I want to separate this place, so method handleRead() could cleanupSelectionKey when the frame size is large but does not reach the MAX_READ_BUFFER_BYTES limit. Then the select method will not cycle through the CPU indefinitely as shown above.
```
if (frameSize > MAX_READ_FRAME_BYTES) {
LOGGER.error("Read a frame size of " + frameSize
+ ", which is bigger than the maximum allowable frame size "
+ MAX_READ_FRAME_BYTES + " for ALL connections.");
return false;
}
// if this frame will push us over the memory limit, then return.
// with luck, more memory will free up the next time around.
if (readBufferBytesAllocated.get() + frameSize > MAX_READ_BUFFER_BYTES) {
return true;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ferrirW commented on a change in pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
ferrirW commented on a change in pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#discussion_r815543315
##########
File path: lib/java/src/org/apache/thrift/server/AbstractNonblockingServer.java
##########
@@ -51,6 +51,7 @@
public static abstract class AbstractNonblockingServerArgs<T extends AbstractNonblockingServerArgs<T>> extends AbstractServerArgs<T> {
public long maxReadBufferBytes = 256 * 1024 * 1024;
+ public long maxReadFrameBytes = 256 * 1024 * 1024;
Review comment:
Just because DEFAULT_MAX_FRAME_SIZE is not exist in 0.9.1.
I will fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
Jens-G commented on pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#issuecomment-1053983980
> static final constant DEFAULT_MAX_FRAME_SIZE is not able for customization.
Right, that's why it is called DEFAULT_xxx and not "current frame size" or the like. It's the default when you don't specify anything else.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G closed pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
Jens-G closed pull request #2533:
URL: https://github.com/apache/thrift/pull/2533
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ferrirW edited a comment on pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
ferrirW edited a comment on pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#issuecomment-1060145988
Actually, I think this problem also exist in the latest version. But I don't have much time to reproduce. So If anyone else is experiencing this problem, they can try use this PR to fix it.
The problem phenomenon:
1. CPU is full
2. Most time consumed on SelectorThread.select()
3. FrameBuffer.read() ends quickly at "if (frameSize > MAX_READ_BUFFER_BYTES))"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ferrirW edited a comment on pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
ferrirW edited a comment on pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#issuecomment-1060145988
Actually, I think this problem also exist in the latest version. But I don't have much time to reproduce. So If anyone else is experiencing this problem, they could try use this PR to fix it.
The problem phenomenon:
1. CPU is full
2. Most time consumed on SelectorThread.select()
3. FrameBuffer.read() ends quickly at "if (frameSize > MAX_READ_BUFFER_BYTES))"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
Jens-G commented on pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#issuecomment-1059558233
Do they also use old log4j versions for the same reason? Anything else vulnerable you want to make publicly known?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ferrirW commented on a change in pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
ferrirW commented on a change in pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#discussion_r815548094
##########
File path: lib/java/src/org/apache/thrift/server/AbstractNonblockingServer.java
##########
@@ -51,6 +51,7 @@
public static abstract class AbstractNonblockingServerArgs<T extends AbstractNonblockingServerArgs<T>> extends AbstractServerArgs<T> {
public long maxReadBufferBytes = 256 * 1024 * 1024;
+ public long maxReadFrameBytes = 256 * 1024 * 1024;
Review comment:
And if you are asking why need this constant:
I want to separate this two judgments, so method handleRead() could cleanupSelectionKey when the frame size is large but does not reach the MAX_READ_BUFFER_BYTES limit. Then the select method will not cycle through the CPU indefinitely as shown above.
```
if (frameSize > MAX_READ_FRAME_BYTES) {
LOGGER.error("Read a frame size of " + frameSize
+ ", which is bigger than the maximum allowable frame size "
+ MAX_READ_FRAME_BYTES + " for ALL connections.");
return false;
}
// if this frame will push us over the memory limit, then return.
// with luck, more memory will free up the next time around.
if (readBufferBytesAllocated.get() + frameSize > MAX_READ_BUFFER_BYTES) {
return true;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
Jens-G commented on pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#issuecomment-1053983075
> if i want use it in 0.9.1, i must
Using 0.9.1 is discouraged anyways, there are a number of CVEs between that version and today. If you make the decision for yourself, that's perfectly fine. But you also live with all the consequences of such decision, because you work on your own fork then. Wouldn't recommend it, but it's a free country, you know.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G removed a comment on pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
Jens-G removed a comment on pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#issuecomment-1053985014
+1 LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ferrirW edited a comment on pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
ferrirW edited a comment on pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#issuecomment-1053882220
> For the sake of consistency, we should aim to use
> DEFAULT_RECURSION_LIMIT = 64;
> DEFAULT_MAX_MESSAGE_SIZE = 100 * 1024 * 1024; // 100 MB
> DEFAULT_MAX_FRAME_SIZE = 16384000; // this value is used consistently across all Thrift libraries
Is it a good idea to use TConfiguration maxFrameSize？
1. static final constant DEFAULT_MAX_FRAME_SIZE is not able for customization.
2. so i have to use trans_.getConfiguration().getMaxFrameSize(), but that means many class need to be modified(TNonblockingSocket, TNonblockingServerSocket, AbstractServerTransportArgs, NonblockingAbstractServerSocketArgs and TEndpointTransport)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ferrirW edited a comment on pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
ferrirW edited a comment on pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#issuecomment-1053882220
> For the sake of consistency, we should aim to use
> DEFAULT_RECURSION_LIMIT = 64;
> DEFAULT_MAX_MESSAGE_SIZE = 100 * 1024 * 1024; // 100 MB
> DEFAULT_MAX_FRAME_SIZE = 16384000; // this value is used consistently across all Thrift libraries
Is it a good idea to use TConfiguration maxFrameSize？
1. static final constant DEFAULT_MAX_FRAME_SIZE is not able for customization.
2. so i have to use trans_.getConfiguration().getMaxFrameSize(), but that means many class need to be modified(TNonblockingSocket, TNonblockingServerSocket, AbstractServerTransportArgs, NonblockingAbstractServerSocketArgs and TEndpointTransport)
3. and if i want use it in 0.9.1, i must add TConfiguration-related code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] ferrirW commented on pull request #2533: THRIFT-5494: fix cpu full caused by infinite select() when frameSize < maxReadBufferBytes but readBufferBytesAllocated.get() + frameSize always greater than MAX_READ_BUFFER_BYTES
Posted by GitBox <gi...@apache.org>.
ferrirW commented on pull request #2533:
URL: https://github.com/apache/thrift/pull/2533#issuecomment-1058875922
> Starting a Gradle Daemon (subsequent builds will be faster)
This failed because AbstractServerTransportArgs.maxFrameSize is not init.
But there seems to be some checks that cannot be pass. Could you please help to check it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org