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

httpd_socket_send return value does not match the docs (IDFGH-9275) #10658

Closed
3 tasks done
wuyuanyi135 opened this issue Jan 30, 2023 · 5 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

@wuyuanyi135
Copy link
Contributor

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.1-dev-3073-g9c29ec0f15

Operating System used.

Windows

How did you build your project?

Command line with idf.py

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

PowerShell

Development Kit.

ESP32-S3-WROOM-1-N16R2

Power Supply used.

USB

What is the expected behavior?

According to the docstring,

* @return

The return value should be either bytes sent (success) or negative error codes. This matches socket send behavior.

What is the actual behavior?

However,

return ESP_ERR_INVALID_ARG;
}
if (!sess->send_fn) {
return ESP_ERR_INVALID_STATE;

will return ESP_ERR_INVALID_ARG or ESP_ERR_INVALID_STATE. They are postive numbers hence confusing the error check whether the return value is less than zero.

Steps to reproduce.

N/A

Debug Logs.

No response

More Information.

Rather than saying this is a documentation issue, I would say this is probably due to inconsistent design of the API about how the error should be reported. In this case, the http_socket_send's return value cannot reliably tell how many bytes were sent. It could only be used to check if any error occurs.

@wuyuanyi135 wuyuanyi135 added the Type: Bug bugs in IDF label Jan 30, 2023
@espressif-bot espressif-bot added the Status: Opened Issue is new label Jan 30, 2023
@github-actions github-actions bot changed the title httpd_socket_send return value does not match the docs httpd_socket_send return value does not match the docs (IDFGH-9275) Jan 30, 2023
@wuyuanyi135
Copy link
Contributor Author

One example of potential problem:

When sending 258 bytes using this API, the return value will match ESP_ERR_INVALID_ARG. If not validating the return value, we are risking sending to a closed socket. However, if validating the return value, we may get a false positive when sending that specific amount of bytes.

@wuyuanyi135
Copy link
Contributor Author

I would recommend instead of returning ESP_ERR_*, the API should return a negative value to ensure the consistency. maybe returning -ESP_ERR_INVALID_ARG and -ESP_ERR_INVALID_STATE

@hmalpani
Copy link
Collaborator

Hello @wuyuanyi135
Thanks for raising the issue. I think instead of returning -ESP_ERR_INVALID_ARG and -ESP_ERR_INVALID_STATE we can return HTTPD_SOCK_ERR_INVALID which is a negative value. This will also match the return values in the API description in the header file.
I will fix this issue internally.

@espressif-bot espressif-bot added Status: In Progress Work is in progress and removed Status: Opened Issue is new labels Jan 31, 2023
@wuyuanyi135
Copy link
Contributor Author

@hmalpani Thanks for the prompt response! FYI, httpd_socket_recv has the same issue and could be fixed similarly.

@hmalpani
Copy link
Collaborator

@hmalpani Thanks for the prompt response! FYI, httpd_socket_recv has the same issue and could be fixed similarly.

Yes. I have raised an internal MR to fix both APIs. The fix will be available soon. Thanks!

@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 1, 2023
espressif-bot pushed a commit that referenced this issue Feb 17, 2023
espressif-bot pushed a commit that referenced this issue Mar 4, 2023
loganfin pushed a commit to Lumenaries/esp_http_server that referenced this issue Apr 23, 2024
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

3 participants