Handle get_object with range #21

windkit · 2015-06-08T04:23:24Z

Instead of using transfer-encoding:chunked, respond with HTTP 200 and resp_body_func
(Note: aws-java-jdk is case-sensitive to HTTP Headers... cowboy internally uses lower case which does not match)

Code is not polished at the moment, I have only tested with a short Java program

Also Unit test is broken at the moment because it checks against transfer-encoding:chunked

yosukehara · 2015-06-09T02:24:43Z

Thank you for your contribution. I've merge this request with feature/fix-issue-376 branch.
I'll test issue-376 on feature/fix-issue-376.

yosukehara · 2015-06-09T02:27:06Z

In addition, I found Leo Gateway could not pass the unit test as below:

leo_gateway_web_tests: range_object_base...*failed*
in function leo_gateway_web_tests:'-range_object_base/2-fun-0-'/2 (test/leo_gateway_web_tests.erl, line 663)
in call from leo_gateway_web_tests:'-range_object_base/2-fun-2-'/2 (test/leo_gateway_web_tests.erl, line 663)
**error:{assertEqual_failed,[{module,leo_gateway_web_tests},
                     {line,663},
                     {expression,"SC"},
                     {expected,206},
                     {value,200}]}


leo_gateway_web_tests: range_object_base...*failed*
in function leo_gateway_web_tests:'-range_object_base/2-fun-0-'/2 (test/leo_gateway_web_tests.erl, line 663)
in call from leo_gateway_web_tests:'-range_object_base/2-fun-2-'/2 (test/leo_gateway_web_tests.erl, line 663)
**error:{assertEqual_failed,[{module,leo_gateway_web_tests},
                     {line,663},
                     {expression,"SC"},
                     {expected,206},
                     {value,200}]}


leo_gateway_web_tests: range_object_base...*failed*
in function leo_gateway_web_tests:'-range_object_base/2-fun-0-'/2 (test/leo_gateway_web_tests.erl, line 663)
in call from leo_gateway_web_tests:'-range_object_base/2-fun-2-'/2 (test/leo_gateway_web_tests.erl, line 663)
**error:{assertEqual_failed,[{module,leo_gateway_web_tests},
                     {line,663},
                     {expression,"SC"},
                     {expected,206},
                     {value,200}]}


cwd:"/Users/yosuke.hara/dev/leo-project/leofs/deps/leo_gateway/.eunit\n"
src/leo_cache_server_dcerl.erl:82:<0.860.0>: Options4 = [{disc_cache_journal_dir,"./cache/journal/"},
            {disc_cache_data_dir,"./cache/data/"},
            {ram_cache_name,mcerl},
            {ram_cache_workers,32},
            {ram_cache_size,128000000},
            {disc_cache_name,dcerl},
            {disc_cache_workers,16},
            {disc_cache_size,128000000},
            {disc_cache_threshold_len,1000000},
            {ram_cache_mod,leo_cache_server_mcerl},
            {disc_cache_mod,leo_cache_server_dcerl},
            {ram_cache_active,true},
            {disc_cache_active,...}]
=======================================================
  Failed: 3.  Skipped: 0.  Passed: 46.

windkit · 2015-06-09T03:33:55Z

Bug fix for End < 0
Updates of Unit test

yosukehara · 2015-06-09T09:30:42Z

@windkit I've checked your request, again and then confirmed the test case. There is no issue. Thanks.

mocchira · 2015-06-10T05:00:43Z

src/leo_gateway_http_commons.erl

+                             get_range_object_1(Req, Bucket, Key, Range, undefined, Socket, Transport)
+                     end, 
+                     Req),
+            ?reply_ok(Header, Req2)


@windkit @yosukehara
Sorry for the late reply to this pull request.

To comply with HTTP spec,
We need to respond HTTP_ST_PARTIAL_CONTENT(206) as a status code so reply_ok can't use.

mocchira · 2015-06-10T05:28:44Z

reopen

windkit · 2015-06-10T06:25:08Z

I have pushed the commits addressing @mocchira comments.

BTW, I am not sure why the CI build is failing... as I can't reproduce it on my machine

mocchira · 2015-06-10T07:11:52Z

@windkit almost LGTM.
There are still two error handling mistakes. I'll comment that on the source view.

BTW, I am not sure why the CI build is failing... as I can't reproduce it on my machine

leo_gateway_web_tests: range_object_base...failed
in function leo_gateway_web_tests:'-range_object_base/2-fun-2-'/2 >>> (test/leo_gateway_web_tests.erl, line 657)
**error:{badmatch,{error,unknown_encoding}}

This error might depend on Erlang version.
Erlang version is specified as R16B03-1 by .travis.yml.
Have you tried with R16B03-1?

mocchira · 2015-06-10T07:14:09Z

src/leo_gateway_http_commons.erl

+        {error, unavailable} ->
+            ?reply_service_unavailable_error([?SERVER_HEADER], Key, <<>>, Req);
+        _ ->
+            ?reply_not_found([?SERVER_HEADER], Key, <<>>, Req)


Need to handle errors of the below clauses

{error, ?ERR_TYPE_INTERNAL_ERROR}

{error, timeout}

mocchira · 2015-06-10T07:39:35Z

src/leo_gateway_http_commons.erl

+                     fun(Socket, Transport) ->
+                             get_range_object_1(Req, Bucket, Key, Range, undefined, Socket, Transport)
+                     end, 
+                     Req),


@windkit I found out why CI failed.
You need to use set_resp_body_fun's another version.

https://github.com/ninenines/cowboy/blob/1.0.0/src/cowboy_req.erl#L776

Try to pass the content length you calculated as the first argument.

Since R16B03-1 coudn't understand transfer-encoding: identify, this error have occured on CI.
https://github.com/erlang/otp/blob/OTP_R16B03-1/lib/inets/src/http_client/httpc_handler.erl#L1122

The latest one can understand this header.
https://github.com/erlang/otp/blob/OTP-17.5.6/lib/inets/src/http_client/httpc_handler.erl#L1130

windkit · 2015-06-10T08:00:37Z

@mocchira Thanks for helping to sort out the cause.
It seems that the only solution is to set the content-length twice, one with explicit Header and one with cowboy
The reason is cowboy sends the header in lower case as 'content-length' but aws-sdk-java assumes it is capitalized as 'Content-Length'...... That's why I did not use the set_resp_body_fun/3 at the first place. So now I would use both....

mocchira · 2015-06-10T08:35:54Z

@windkit YW and GJ!

It seems that the only solution is to set the content-length twice, one with explicit Header and one with cowboy

Yes.
Unfortunately there are lots of aws client implementations which do NOT comply with RFC2616.
So we need to handle those clients not only aws-sdk :(
That's why capitalized http headers are defined in leo_http.hrl.

mocchira · 2015-06-10T08:44:09Z

@yosukehara
Finished to review and @windkit have fixed all issues I pointed out,
so please merge if you don't have something to be

windkit · 2015-06-11T08:43:06Z

And setting both actually breaks other clients such as boto (Python).......

yosukehara · 2015-06-11T15:14:31Z

I've merged your code with the branch of feature/fix-issue-376 as follows:

7732015?diff=split

windkit · 2015-06-12T01:44:26Z

Short Summary of setting content-length (pass length to cowboy:set_reps_body_fun/3) and Content-Length (Explicitly set the HTTP Header) in HTTP Header

Setting both, breaks
aws-sdk-ruby (both v1 and v2)
boto

Setting content-length only, breaks
aws-sdk-ruby (both v1 and v2)
boto
aws-sdk-java
(RFC suggests headers are case-insensitive but actually it is implementation dependent)
[Before reading the RFC, I also thought it should be Content-Length ...]

Setting Content-Length only, breaks
httpc in Erlang R16B03-1
(Technically Speaking, it is about cowboy setting content-type to identity if content length is not specified in sending the Reply HTTP Header and in R16B03-1, it was not handled)

mocchira · 2015-06-12T06:35:21Z

@windkit Thank you for the Good summary.

So Applying the patch which replace <<"content-length">> at this line https://github.com/ninenines/cowboy/blob/1.0.0/src/cowboy_req.erl#L869 with <<"Content-Length">> will solve all issues?
if so, I think we should go with this patch.

windkit · 2015-06-15T03:47:14Z

@mocchira The fix seems working, the issue I encountered after applying the patch was related to leo_storage leo-project/leofs#382

windkit · 2015-06-15T09:06:47Z

Tested with the simple "Content-Length" Hack, works fine

mocchira · 2015-06-15T09:11:13Z

LGTM.

yosukehara · 2015-06-15T09:23:35Z

I've added other fixed code: leo-project/cowboy@22a4dcf
OK then, we will check this issue more.

yosukehara · 2015-06-15T09:37:54Z

I've merged your code with feature/fix-issue-376 branch as follows:

merged code: 5e82aa0

windkit added 3 commits June 8, 2015 10:38

Changed to reply 200 for range_get

d9aefc3

Added Content-Length

22f1c2b

Code Refactoring

b3adfac

windkit force-pushed the range_get branch from ad9358e to b3adfac Compare June 8, 2015 06:36

windkit added 2 commits June 9, 2015 12:33

Bug fix for End < 0

fd5d13f

Updates Unit Test

789478f

yosukehara closed this Jun 9, 2015

mocchira reviewed Jun 10, 2015
View reviewed changes

mocchira reopened this Jun 10, 2015

Fixed Reply HTTP 206, categorized error types

d52c9b2

windkit force-pushed the range_get branch 2 times, most recently from 30e3d0b to e7e7c38 Compare June 10, 2015 06:24

mocchira reviewed Jun 10, 2015
View reviewed changes

Added Error Handling to Range Get

6d85afd

windkit force-pushed the range_get branch from e7e7c38 to 6d85afd Compare June 10, 2015 07:24

mocchira reviewed Jun 10, 2015
View reviewed changes

Fix for R16B03

33a3407

windkit force-pushed the range_get branch from 4f480e8 to f8c5b3c Compare June 15, 2015 00:58

windkit added 2 commits June 15, 2015 17:41

Use set_resp_body_fun/3 for Content Length, Require Patch to cowboy

c6aaa66

Switched to use forked cowboy

794856c

windkit force-pushed the range_get branch from ceaf778 to 794856c Compare June 15, 2015 08:47

yosukehara merged commit 794856c into leo-project:develop Jun 16, 2015

yosukehara mentioned this pull request Jun 17, 2015

[leo_gateway] Cannot get an object with range leo-project/leofs#376

Closed

windkit deleted the range_get branch March 2, 2016 03:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle get_object with range #21

Handle get_object with range #21

windkit commented Jun 8, 2015

yosukehara commented Jun 9, 2015

yosukehara commented Jun 9, 2015

windkit commented Jun 9, 2015

yosukehara commented Jun 9, 2015

mocchira Jun 10, 2015

mocchira commented Jun 10, 2015

windkit commented Jun 10, 2015

mocchira commented Jun 10, 2015

mocchira Jun 10, 2015

mocchira Jun 10, 2015

windkit commented Jun 10, 2015

mocchira commented Jun 10, 2015

mocchira commented Jun 10, 2015

windkit commented Jun 11, 2015

yosukehara commented Jun 11, 2015

windkit commented Jun 12, 2015

mocchira commented Jun 12, 2015

windkit commented Jun 15, 2015

windkit commented Jun 15, 2015

mocchira commented Jun 15, 2015

yosukehara commented Jun 15, 2015

yosukehara commented Jun 15, 2015

Handle get_object with range #21

Handle get_object with range #21

Conversation

windkit commented Jun 8, 2015

yosukehara commented Jun 9, 2015

yosukehara commented Jun 9, 2015

windkit commented Jun 9, 2015

yosukehara commented Jun 9, 2015

mocchira Jun 10, 2015

Choose a reason for hiding this comment

mocchira commented Jun 10, 2015

windkit commented Jun 10, 2015

mocchira commented Jun 10, 2015

mocchira Jun 10, 2015

Choose a reason for hiding this comment

mocchira Jun 10, 2015

Choose a reason for hiding this comment

windkit commented Jun 10, 2015

mocchira commented Jun 10, 2015

mocchira commented Jun 10, 2015

windkit commented Jun 11, 2015

yosukehara commented Jun 11, 2015

windkit commented Jun 12, 2015

mocchira commented Jun 12, 2015

windkit commented Jun 15, 2015

windkit commented Jun 15, 2015

mocchira commented Jun 15, 2015

yosukehara commented Jun 15, 2015

yosukehara commented Jun 15, 2015