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

Fix binary and chunked HTTP downloads. Add diffs from @anod221. #2985

Merged

Conversation

HHHartmann
Copy link
Member

Fixes #2405.

  • This PR is for the dev branch rather than for master.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.

Checking the sources that @anod221 gave I created these diffs.
They look ok but I did not have a chance to test them yet.

They also fix an issue with chunked encoding replies which I also have not tested yet.

Checking the sources that @anod221 gave I created these diffs.
They look ok but I did not have a chance to test them yet.
@wtarreau
Copy link

Hi!
I've just tested it now with the same method as in #3036, and it works fine, it now properly reports all downloaded bytes and does not stop at \0 anymore:

> http.get("http://10.9.0.22:8080/lfs.img","range: bytes=0-1023\r\n",function(code,data,headers) print(code,headers["content-length"],#data) end)
> 206   1024    1024

A large response still causes a panic though if I omit the range:

> http.get("http://10.9.0.22:8080/lfs.img","",function(code,data,headers) print(code,headers["content-length"],#data) end)
> HTTP client: Response too long (8761)
PANIC: unprotected error in call to Lua API (stdin:1: attempt to index a nil value)

but my understanding is that it was more or less expected anyway, and nothing prevents us from downloading large images 1kB at a time using range requests.

@HHHartmann
Copy link
Member Author

HHHartmann commented Apr 25, 2020

@wtarreau thats great news, Thaks for testing this.
I was pretty sure I wrote the message I am writing now before. Don't know why it is'nt shown here.

In your example ending up in a PANIC I suspect that the PANIC is either from accessing headers or data parameters to the callback which will be nil in case of an error.
The message seems to be printed by the c code. (haven't checked in the code though)

Do you have a chance to also check the chunked transfer part?

@wtarreau
Copy link

wtarreau commented Apr 25, 2020 via email

@marcelstoer marcelstoer changed the title WIP: fix binary and chunked http downloads. Add diffs from @anod221. WIP: Fix binary and chunked HTTP downloads. Add diffs from @anod221. May 7, 2020
@marcelstoer
Copy link
Member

@wtarreau any news?

@wtarreau
Copy link

wtarreau commented May 7, 2020

Hi Marcel. Seems like it's the second time I have to tell you I feel ashamed. I knew I had something to do last week-end and didn't remember what, given how long my todo list was and that I forgot to note it :-/ Now I've written it on my todo-list hoping I'll do it this week-end. Sorry again :-/

@marcelstoer
Copy link
Member

No worries. Don't be too hard on yourself. There are many other developers in our community - including yours truly - who could have tested this as well.

@wtarreau
Copy link

wtarreau commented May 8, 2020

Hi Marcel,

so the test is conclusive, it works. I also verified that prepending zeroes in front of the chunk sizes worked as expected:

server$ printf "HTTP/1.1 200 OK\r\nTransfer-encoding: chunked\r\nConnection: close\r\n\r\n1\r\nA\r\n02\r\nBC\r\n003\r\nDEF\r\n0\r\n\r\n" | nc6 --half-close -lp8181
GET /foo HTTP/1.1
Host: 10.9.0.22:8181
Connection: close
User-Agent: ESP8266
> http.get("http://10.9.0.22:8181/foo",nil,function(code,data,headers) print(code,#data,data) end)
200   6       ABCDEF

I had met an issue with panics when the server is not listening that caused me some gray hair, until I figured that it was my "print(#data)" which used to trigger the panic when data is nil, so everything's OK here in fact :-)

Thus for me the patch is both valid and useful, it will be very welcome!

@marcelstoer marcelstoer changed the title WIP: Fix binary and chunked HTTP downloads. Add diffs from @anod221. Fix binary and chunked HTTP downloads. Add diffs from @anod221. May 10, 2020
@marcelstoer marcelstoer merged commit 6e5edf4 into nodemcu:dev May 10, 2020
@marcelstoer
Copy link
Member

Thanks Willy!

vsky279 pushed a commit to vsky279/nodemcu-firmware that referenced this pull request May 23, 2020
marcelstoer pushed a commit that referenced this pull request Jun 9, 2020
@HHHartmann HHHartmann deleted the http-binary-and-chunked-handling branch January 3, 2021 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants