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

ESP HTTP Client: disable_auto_redirect causes infinite loop (IDFGH-9243) #10629

Closed
3 tasks done
jgillick opened this issue Jan 27, 2023 · 3 comments
Closed
3 tasks done
Assignees
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally Type: Bug bugs in IDF

Comments

@jgillick
Copy link

Answers checklist.

  • I have read the documentation ESP-IDF Programming Guide and the issue is not addressed there.
  • I have updated my IDF branch (master or release) to the latest version and checked that the issue is present there.
  • I have searched the issue tracker for a similar issue and not found a similar issue.

IDF version.

v5.0

Operating System used.

macOS

How did you build your project?

Command line with idf.py

If you are using Windows, please specify command line type.

None

Development Kit.

XIAO_ESP32C3

Power Supply used.

USB

What is the expected behavior?

When using .disable_auto_redirect = true, I'd expect the HTTP request to end when it get's to a redirect. At that point, the firmware can manually get the location header and decide what to do with the redirect or handle it in the event handler.

esp_http_client_config_t config = {
    .url = "http://httpbin.org/absolute-redirect/3",
    .event_handler = _http_event_handler,
    .disable_auto_redirect = true,
};

What is the actual behavior?

The HTTP client goes into an infinite loop until redirect_counter reaches the maximum count.

This is caused by the following lines of code:

if (client->disable_auto_redirect) {
    http_dispatch_event(client, HTTP_EVENT_REDIRECT, NULL, 0);
} else {
    ESP_ERROR_CHECK(esp_http_client_set_redirection(client));
}
client->redirect_counter ++;
client->process_again = 1;

The block of code prevents the redirect from happening, but then sets process_again to 1, which causes the response to be processed again.

I believe the solution should be to put the redirect_counter and process_again assignments in the else statement.

if (client->disable_auto_redirect) {
    ESP_LOGI(TAG, "REDIRECT!");
    http_dispatch_event(client, HTTP_EVENT_REDIRECT, NULL, 0);
} else {
    ESP_ERROR_CHECK(esp_http_client_set_redirection(client));
    client->redirect_counter ++;
    client->process_again = 1;
}

Steps to reproduce.

  1. Create a simple HTTP client that points to a URL with a redirect and set disable_auto_redirect to false.
  2. Do NOT handle the redirect in the event handler
  3. Run the program, and observe that the request hits the maximum redirects error and fails.

Debug Logs.

No response

More Information.

No response

@jgillick jgillick added the Type: Bug bugs in IDF label Jan 27, 2023
@espressif-bot espressif-bot added the Status: Opened Issue is new label Jan 27, 2023
@github-actions github-actions bot changed the title ESP HTTP Client: disable_auto_redirect causes infinite loop ESP HTTP Client: disable_auto_redirect causes infinite loop (IDFGH-9243) Jan 27, 2023
@espressif-bot espressif-bot added Status: In Progress Work is in progress and removed Status: Opened Issue is new labels Jan 30, 2023
@AxelLin
Copy link
Contributor

AxelLin commented Feb 14, 2023

master: 5feb469
v5.0: 9463a66

@Harshal5
Copy link
Collaborator

Hello @jgillick ,
Sorry for the delayed response.

The HTTP client goes into an infinite loop until redirect_counter reaches the maximum count.

Thank you for reporting this issue. Rather than putting the redirect_counter and process_again assignments in the else statement, we should add them in the esp_http_client_set_redirection function, because it will also handle the case when disable_auto_redirect = true and the user calls esp_http_client_set_redirection in the event handler.

Attached is the patch for same. Could you please check if it satisfies your requirement?
0001-http_client-fixed-looping-caused-when-disable_auto_r.patch

@jgillick
Copy link
Author

@Harshal5 I haven't had a chance to try this patch directly, but the change makes sense and looks good.

@espressif-bot espressif-bot added Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: In Progress Work is in progress Resolution: NA Issue resolution is unavailable labels Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally Type: Bug bugs in IDF
Projects
None yet
Development

No branches or pull requests

4 participants