search

esp-rs / esp-idf-svc

Type-Safe Rust Wrappers for various ESP-IDF services (WiFi, Network, Httpd, Logging, etc.)
https://docs.esp-rs.org/esp-idf-svc/
Apache License 2.0
333 stars 183 forks source link

Flush responses to avoid repeated request failures #404

Closed weiying-chen closed 8 months ago

weiying-chen commented 8 months ago

Fixes #399.

I tested by using a loop that sends many HTTPS requests and by removing the part that reads and drains the data in the http_client.rs example. The pattern was the same: ESP_FAIL without flush_response(). No ESP_FAIL with flush_response().

Some considerations:

  1. Will flushing the response data in initiate_request() cause issues that cannot be observed in the http_client.rs example?
  2. If the response data is flushed in initiate_request(), will flushing the response data in fetch_headers() be redundant?
Vollbrecht commented 8 months ago

Thanks for the PR!

Some considerations:

1. Will flushing the response data in `initiate_request()` cause issues that cannot be observed in the `http_client.rs` example?

2. If the response data is [flushed](https://github.com/weiying-chen/esp-idf-svc/blob/feature/flush-response/src/http/client.rs#L213) in `initiate_request()`, will [flushing](https://github.com/weiying-chen/esp-idf-svc/blob/feature/flush-response/src/http/client.rs#L452) the response data in `fetch_headers()` be redundant?

Yeah that are some good questions. Potentially we have 3 states (new, request, response). Since you made the additional method public we need to know any site-effects on calling it in any of the 3 states, e.g it dosn't brake anything when its called randomly.

Also what is happening if you call flush_response() repeatedly? Is it save to do?

Does flushing len 0 in this context works as expeted, 0 meaning flush everything you have ?

weiying-chen commented 8 months ago

Yeah that are some good questions. Potentially we have 3 states (new, request, response). Since you made the additional method public we need to know any site-effects on calling it in any of the 3 states, e.g it dosn't brake anything when its called randomly.

Also what is happening if you call flush_response() repeatedly? Is it save to do?

Does flushing len 0 in this context works as expeted, 0 meaning flush everything you have ?

Those are good questions too. I didn't think of them. I'll try to test each of those items and get back to you!

weiying-chen commented 8 months ago

I'm back.

  1. No ESP_FAIL if you run flush_response() repeatedly.

  2. No ESP_FAIL if you run flush_response() in a public context (e.g. in http_client.rs).

  3. No ESP_FAIL if you run flush_response() when State is New and Response. There is ESP_FAIL if State is Request. But I guess no one is going to run flush_response() in the last situation? Or maybe we should add a warning or if statement?

Note: when there was no ESP_FAIL, the code ran successfully.

weiying-chen commented 8 months ago

I think if we can clarify and fix the last two bits i asked down below we are good two go !

Okay, let me take a look and come back to you!

Vollbrecht commented 8 months ago

I think if we can clarify and fix the last two bits i asked down below we are good two go !

Okay, let me take a look and come back to you!

One last thing can you make sure that the http_client,rs example also still works with the changes !

Vollbrecht commented 8 months ago

Thanks for working on this!

The http_client example still worked for me with the changes.

weiying-chen commented 8 months ago

Thanks for working on this!

I use this crate a lot. So I'm happy to help!