search

ukhas / js-logtail

Remote log tailing using the HTTP Range header.
109 stars 54 forks source link

Refactor a little #9

Closed danielrichman closed 9 years ago

danielrichman commented 9 years ago

Includes and subsumes #8

rossengeorgiev commented 9 years ago

Another thing replace:

xhr.getResponseHeader("Content-Length");

with

parseInt(xhr.getResponseHeader("Content-Length"));

If we get Content-Length header more than once, getReponseHeader will return "100, 100" and when we do (log_file_size - 1) we get NaN and everything is sad.

danielrichman commented 9 years ago

I think this is correct "behaviour" in the sense that I'm pretty sure (?) a Content-Length of "100, 100" / several content length headers is invalid and should be rejected.

Note that if we get "100, 100" it will fail the isNaN check immediately below.

However, I do agree that we should explicitly convert to integers. I intend to use this idiom:

var x = +y;

instead of parseInt, because parseInt will parse "15asdfasdf" as 15.

danielrichman commented 9 years ago

Oh, but +s will convert floats; "123456.123". :-( MDN suggests REGEXing the string before using parseInt

rossengeorgiev commented 9 years ago

Varnish does that. We get 100, 100 and we only need the first one, so parseInt() is fine. It will stop at the ,.

2015-03-02_mtjurd6710

danielrichman commented 9 years ago

I disagree. This is a problem I think we should first look at fixing in Varnish, rather than hacking around it in JS: it is not legal to send Content-Length twice. Is this caused by the do_stream thing? Perhaps it is not meant to be used in that way? (Though even if it is not this is pretty dumb/weird on Varnish's part)

rossengeorgiev commented 9 years ago

Oh I agree. Varnish is broken and needs to be updated. Using do_stream is a hack to avoid the negative low end on content-range.

https://www.varnish-cache.org/trac/ticket/1323

Should be fixed in v4

danielrichman commented 9 years ago

OK to merge this then?