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

[3.2] http-max-bytes-in-flight-mb fixes #59

Closed
wants to merge 1 commit into from

Conversation

linh2931
Copy link
Member

Resolve #48

  • allow http-max-bytes-in-flight-mb bigger than 4GB
  • make -1 indicate unlimited
  • add range check

allow http-max-bytes-in-flight-mb bigger than 4GB; make -1 indicate unlimited; add range check
@@ -217,7 +217,7 @@ class http_plugin_impl : public std::enable_shared_from_this<http_plugin_impl> {
std::optional<eosio::chain::named_thread_pool> thread_pool;
std::atomic<size_t> bytes_in_flight{0};
std::atomic<int32_t> requests_in_flight{0};
size_t max_bytes_in_flight = 0;
uint64_t max_bytes_in_flight = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is later compared with a size_t, I think it is clearer as a size_t.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I will change back to size_t (was thinking make it explicitly 64 bits)

@@ -776,7 +776,16 @@ class http_plugin_impl : public std::enable_shared_from_this<http_plugin_impl> {
EOS_ASSERT( my->thread_pool_size > 0, chain::plugin_config_exception,
"http-threads ${num} must be greater than 0", ("num", my->thread_pool_size));

my->max_bytes_in_flight = options.at( "http-max-bytes-in-flight-mb" ).as<uint32_t>() * 1024 * 1024;
// http-max-bytes-in-flight-mb is allowed to be -1,
// that's why int64_t is used
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would not mention this except I have the other comment. This could be on the same line or even just remove the second line of the comment.

EOS_ASSERT( max_bytes_mb == -1 || (max_bytes_mb >= 0 && max_bytes_mb < std::numeric_limits<int64_t>::max() / (1024 * 1024)), chain::plugin_config_exception,
"http-max-bytes-in-flight-mb ${max_bytes_mb} must be equal to or greater than 0 and less than ${max}, or -1", ("max_bytes_mb", max_bytes_mb) ("max", std::numeric_limits<int64_t>::max() / (1024 * 1024)) );
if ( max_bytes_mb == -1 ) {
my->max_bytes_in_flight = std::numeric_limits<uint64_t>::max();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should change to size_t as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will change.

@spoonincode
Copy link
Member

This arguably could be a 3.1 candidate.

@linh2931
Copy link
Member Author

If we do this for 3.1, should I close this PR, do 3.1 first, and then merge 3.1 to main?

@spoonincode
Copy link
Member

I'm on the fence about it. #48 does make it sound like it could be treated as a defect since setting it to a high value silently ends up breaking HTTP by not behaving as expected. I'm merely suggesting it could be considered for 3.1 I'm not sure I have a strong feeling either way.

@linh2931
Copy link
Member Author

Since it is not a big effort and prevents a potentially broken HTTP, I will do a 3.1 and then main.

@linh2931
Copy link
Member Author

Close this. Use #64 instead, which is merged from the fix in release 3.1 #63.

@linh2931 linh2931 closed this Aug 28, 2022
@linh2931 linh2931 deleted the http_bytes_in_flight_fix branch August 28, 2022 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

http-max-bytes-in-flight-mb = 4096 fails, should generate config error on nodeos startup
3 participants