Skip to content

Conversation

gustavowd
Copy link

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • [ x ] I have used cargo fmt command to ensure that all changed code is formatted correctly.
  • [ x ] I have used cargo clippy command to ensure that all changed code passes latest Clippy nightly lints.
  • [ x ] My changes were added to the CHANGELOG.md in the proper section.

Pull Request Details 📖

Simple add an option the enable the keep alive configuration of the http client.

Description

Due to not enable the keep alive, sometimes the connection timed out, bringing several issues to the upper layer functions.

Testing

I tested the new client with REST API requests.

@@ -86,6 +86,7 @@ pub struct Configuration {
pub use_global_ca_store: bool,
pub crt_bundle_attach: Option<unsafe extern "C" fn(conf: *mut core::ffi::c_void) -> esp_err_t>,
pub raw_request_body: bool,
pub keep_alive_enable: Option<bool>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does None mean for keep_alive_enabled? If it means same as Some(false), then why are we hiding this flag behind an Option?

Copy link
Author

Choose a reason for hiding this comment

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

What does None mean for keep_alive_enabled? If it means same as Some(false), then why are we hiding this flag behind an Option?

In fact, None would cause the http client library to use the default option for keep_alive_enabled, which is currently false. However, if the library changes in the future and starts specifying this option as true by default, None would maintain this condition.

In any case, I have no objection to removing the Option and specifying just a boolean.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is simpler for the users to just use false.

Moreover, there is no such thing as a predefined default in esp-idf that might change. The C struct is initially just all zeroes, which for that particular parameter corresponds to false.

Copy link
Author

Choose a reason for hiding this comment

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

@ivmarkov

Actually, there is a default configuration for keep alive if you set in the config keep_alive_enable to true.

static bool init_common_tcp_transport(esp_http_client_handle_t client, const esp_http_client_config_t *config, esp_transport_handle_t transport)
{
    if (config->keep_alive_enable == true) {
        client->keep_alive_cfg.keep_alive_enable = true;
        client->keep_alive_cfg.keep_alive_idle = (config->keep_alive_idle == 0) ? DEFAULT_KEEP_ALIVE_IDLE : config->keep_alive_idle;
        client->keep_alive_cfg.keep_alive_interval = (config->keep_alive_interval == 0) ? DEFAULT_KEEP_ALIVE_INTERVAL : config->keep_alive_interval;
        client->keep_alive_cfg.keep_alive_count =  (config->keep_alive_count == 0) ? DEFAULT_KEEP_ALIVE_COUNT : config->keep_alive_count;
        esp_transport_tcp_set_keep_alive(transport, &client->keep_alive_cfg);
    }

Maybe should be interesting to have the keep_alive_enabled just as a boolean and the other configurations as Option<>.
What do you think about this?

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.

2 participants