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

http.get closes before end of data from v1.97 #1458

Closed
sirpy opened this issue Jun 5, 2018 · 14 comments
Closed

http.get closes before end of data from v1.97 #1458

sirpy opened this issue Jun 5, 2018 · 14 comments

Comments

@sirpy
Copy link

sirpy commented Jun 5, 2018

on ESP8266
I'm trying to download a 4k file via http.get
on v1.96 it works just fine, the res.on(data) is called multiple times and the whole file is downloaded
from v1.97-v1.99 res.on(data) is called only once with around 400bytes and then res.on(close) is called, so the file is not downloaded correctly.

@gfwilliams
Copy link
Member

@opichals ? Could this be related to the HTTP chunked stuff again?

@wilberforce
Copy link
Member

@sirpy

Please can you provide a simple test case?

Is the esp8266 only or does this occur on the Linux build too?

@opichals
Copy link
Contributor

opichals commented Jun 5, 2018

@gfwilliams Could be.
@sirpy Please provide a test case.

There is test_http_post_chunked.js and other test_http*.js but perhaps that is not sufficient.

@sirpy
Copy link
Author

sirpy commented Jun 5, 2018

seems like it only happens on my nodejs server, it doesnt happen with local apache server.
The only thing i found different is that nodejs/expressjs uses lowercase response headers.
so if the http client checks for case sensitive headers it might be the issue.

@opichals
Copy link
Contributor

opichals commented Jun 6, 2018

@sirpy Yeah, the HTTP headers letter case handling is not addressed.

Perhaps we could convert to lowercase in the socketserver.c. It would break existing header handling code though. An alternative lowercase headers lookup object could be added and later we could deprecate the current exact casing support.

I'd be nice to rather have a special Object type for the case-insensitive key lookup. @gfwilliams ?

@gfwilliams
Copy link
Member

@sirpy please could you hack up some simple node.js server code that lets us reproduce the issue here?

@opichals it seems that Node.js does lowercase them so I guess it'd be more compliant - however I can pretty much guarantee I'll get complaints if we mess with the case of the headers that get reported back - there'll be a bunch of people's code out there that relies on it (I can see at least 3 things in EspruinoDocs, one of which is the WebSockets lib).

I guess the easiest solution for now is to make a jsvObjectGetChildIgnoreCase that does a case-insensitive search, and then to use that? I'm happy to do that but I'd really like to have a way of testing that I've fixed the issue first.

I'm still a bit confused about the HTTP Chunking - it never seemed to be an issue before, but when the client started reporting HTTP/1.1 did some servers start using it in their responses?

@gfwilliams
Copy link
Member

... having said that about headers, maybe it's better to do it now rather than later :) I guess it's not too bad to change the code that uses it to headers["Content-Type"]||headers["content-type"]

@opichals
Copy link
Contributor

opichals commented Jun 6, 2018

I'm sure we could find multiple use-cases for a case-insensitive Object lookup. I'd rather do that and allow users to use that from their JS code too (instead of doing the jsvObjectGetChildIgnoreCase).

@gfwilliams
Copy link
Member

instead of doing the jsvObjectGetChildIgnoreCase

Not sure I understand?

I'm sure we could find multiple use-cases for a case-insensitive Object lookup

Amazingly this doesn't seem to be part of basic JS. I guess we could expose it via an E.lookupNoCase sort of function?

@opichals
Copy link
Contributor

opichals commented Jun 6, 2018

E.lookupNoCase sounds great!

@gfwilliams
Copy link
Member

@sirpy please can you try the latest travis builds out and see if this fixes your issue

@sirpy
Copy link
Author

sirpy commented Jun 6, 2018 via email

@opichals
Copy link
Contributor

opichals commented Jun 6, 2018

@gfwilliams I'd be nice to make the http request/response headers objects actually be a Proxy (just added to the ES* wishlist) as follows by default:

let headers = {
  'Content-Type': 'text/plain'
};
headers = new Proxy(headers, { get: E.lookupNoCase });

console.log(headers['content-type']);
// output: "text/plain"
console.log(Object.keys(headers));
// output: [ "Content-Type" ]

This way the node.js targetted code would just work in Espruino as the lowercase lookups would be ok.

@sirpy
Copy link
Author

sirpy commented Jun 6, 2018

works great. thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants