arduino-libraries / ArduinoHttpClient

Arduino HTTP Client library
282 stars 170 forks source link

Make sure to use HttpClient when upgrading the connection for websockets #152

Closed tim-vandecasteele closed 5 months ago

tim-vandecasteele commented 1 year ago

disclaimer: I'm a bit puzzled by this problem, as everyone should have this problem but then people would have a lot of problems with websockets

Context: I'm using websockets over https, using WiFiClientSecure

Using WebSocketClient::begin I got into problems where status = responseStatusCode(); would be trying to read the HTTP header, but because both HttpClient and WebSocketClient have a read function, the read from WebSocketClient was used, which returns a bunch of gibberish bytes. This caused the WebSocket to think that the connection was not successfully upgraded, whereas in reality the webserver gave a proper response.

websocketClient = new WebSocketClient(wifiWebsocket, "www.host.com", 443);
int ret = websocketClient->begin(websocketPath);

this basically gave ret = -4, because responseStatusCode couldn't find the HTTP header.

Let me know what the structure is to get things merged, or what adaptations are required.

(I tested this on a simple esp32 board)

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

github-actions[bot] commented 1 year ago

Memory usage change @ 8566fb4cc0098ef2c91f080f5eb877eefa0e23b6

Board flash % RAM for global variables %
arduino:samd:mkr1000 :green_heart: -8 - 0 -0.0 - 0.0 0 - 0 0.0 - 0.0
Click for full report table Board|`examples/BasicAuthGet`
flash|%|`examples/BasicAuthGet`
RAM for global variables|%|`examples/CustomHeader`
flash|%|`examples/CustomHeader`
RAM for global variables|%|`examples/DweetGet`
flash|%|`examples/DweetGet`
RAM for global variables|%|`examples/DweetPost`
flash|%|`examples/DweetPost`
RAM for global variables|%|`examples/HueBlink`
flash|%|`examples/HueBlink`
RAM for global variables|%|`examples/PostWithHeaders`
flash|%|`examples/PostWithHeaders`
RAM for global variables|%|`examples/SimpleDelete`
flash|%|`examples/SimpleDelete`
RAM for global variables|%|`examples/SimpleGet`
flash|%|`examples/SimpleGet`
RAM for global variables|%|`examples/SimpleHttpExample`
flash|%|`examples/SimpleHttpExample`
RAM for global variables|%|`examples/SimplePost`
flash|%|`examples/SimplePost`
RAM for global variables|%|`examples/SimplePut`
flash|%|`examples/SimplePut`
RAM for global variables|%|`examples/SimpleWebSocket`
flash|%|`examples/SimpleWebSocket`
RAM for global variables|% -|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|-|- `arduino:samd:mkr1000`|0|0.0|0|0.0|0|0.0|0|0.0|-8|-0.0|0|0.0|0|0.0|0|0.0|-8|-0.0|0|0.0|0|0.0|0|0.0|-8|-0.0|0|0.0|0|0.0|0|0.0|0|0.0|0|0.0|0|0.0|0|0.0|0|0.0|0|0.0|0|0.0|0|0.0
Click for full report CSV ``` Board,examples/BasicAuthGet
flash,%,examples/BasicAuthGet
RAM for global variables,%,examples/CustomHeader
flash,%,examples/CustomHeader
RAM for global variables,%,examples/DweetGet
flash,%,examples/DweetGet
RAM for global variables,%,examples/DweetPost
flash,%,examples/DweetPost
RAM for global variables,%,examples/HueBlink
flash,%,examples/HueBlink
RAM for global variables,%,examples/PostWithHeaders
flash,%,examples/PostWithHeaders
RAM for global variables,%,examples/SimpleDelete
flash,%,examples/SimpleDelete
RAM for global variables,%,examples/SimpleGet
flash,%,examples/SimpleGet
RAM for global variables,%,examples/SimpleHttpExample
flash,%,examples/SimpleHttpExample
RAM for global variables,%,examples/SimplePost
flash,%,examples/SimplePost
RAM for global variables,%,examples/SimplePut
flash,%,examples/SimplePut
RAM for global variables,%,examples/SimpleWebSocket
flash,%,examples/SimpleWebSocket
RAM for global variables,% arduino:samd:mkr1000,0,0.0,0,0.0,0,0.0,0,0.0,-8,-0.0,0,0.0,0,0.0,0,0.0,-8,-0.0,0,0.0,0,0.0,0,0.0,-8,-0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0 ```
tim-vandecasteele commented 1 year ago

Hi @per1234 are you still maintaining this project? If so, let me know if there's anything else required, if not, do you prefer that people fork this, or is there a process to find a new maintainer?

per1234 commented 1 year ago

Hi @tim-vandecasteele. Thanks for your pull request!

My role in this project is janitorial. I try to help keep the issue tracker tidy and maintain the infrastructure, but am not in a role to decide on changes to the codebase.

One of the other maintainers will need to take care of the review and merging of this pull request.

tim-vandecasteele commented 1 year ago

Hi, sorry, I was too quick drawing conclusions from the dependabot merges. Any idea who would be a maintainer for this project then? Looking through the commits there weren't a lot of merges other than these updates in this project.

Sorry to bring these questions to you, not really clear who else should be addressed 😅

andreagilardoni commented 6 months ago

Hi @tim-vandecasteele, I would also add HttpClient:: on this line https://github.com/arduino-libraries/ArduinoHttpClient/blob/5c5fafb4e8b279b0041d039b8cb5571ffdf624f2/src/HttpClient.cpp#L140 As far as I am aware I cannot suggest a change on that line. I don't see these changes harmful for the repository, I will try them and merge this PR. Thanks for your work!