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

Add Transport:sendfile/4,/5 #41

Merged
merged 1 commit into from Aug 27, 2013

Conversation

Projects
None yet
2 participants
@fishcakez
Contributor

fishcakez commented Mar 29, 2013

Adds sendfile/4 and sendfile/5 to ranch_tcp and ranch_ssl, and sendfile/2,/4,/5 to ranch_transport. There is a random property based test, done by hand - didn't want to add a dependency though could use a property testing lib.

I believe this relates to: ninenines/cowboy#306 ninenines/cowboy#313 #22

Edit: I've amended the commit to remove unnecessary file:position calls in ranch_ssl:sendfile/2 (and /4,/5) that the previous version of this commit had introduced.

@fishcakez

This comment has been minimized.

Contributor

fishcakez commented Mar 29, 2013

I believe that ranch_tcp:sendfile/5 will have the same bug as #39.

@fishcakez

This comment has been minimized.

Contributor

fishcakez commented Apr 2, 2013

This is missing the test/acceptor_SUITE_data/sendfile file. I will added it when at laptop. It is just a 1kb file with random bytes.

@fishcakez

This comment has been minimized.

Contributor

fishcakez commented Apr 2, 2013

Rebased and includes missing data file. Note we now pass sendfile/2 to sendfile/4. The file module does this too. This made me realise the comment above ranch_tcp:sendfile/2 is incorrect. It is an efficient "read and write", not "open and write".

Should I remove the try..after..file:close/1 and just do file:close/1 as this is file:sendfile/5 fallback behaviour?

@essen

This comment has been minimized.

Member

essen commented Apr 8, 2013

It all feels so complicated. I have a hard time wrapping my head around it.

@fishcakez

This comment has been minimized.

Contributor

fishcakez commented Apr 8, 2013

Sure I understand that, I will have a look at this before the weekend.

@essen

This comment has been minimized.

Member

essen commented Jun 19, 2013

I'll be looking into merging this in the next few days. Looks like it needs rebasing, use of ct_helper for certificate and perhaps more.

I'm only catching up on new tickets for now so will take a better look afterwards.

@fishcakez

This comment has been minimized.

Contributor

fishcakez commented Jun 19, 2013

I'll rebase/ct_helper today :).

On 19 June 2013 14:45, Loïc Hoguin notifications@github.com wrote:

I'll be looking into merging this in the next few days. Looks like it
needs rebasing, use of ct_helper for certificate and perhaps more.

I'm only catching up on new tickets for now so will take a better look
afterwards.


Reply to this email directly or view it on GitHubhttps://github.com//pull/41#issuecomment-19684648
.

@fishcakez

This comment has been minimized.

Contributor

fishcakez commented Jun 20, 2013

  • Rebased / add ct_helper.
  • Move ranch_ssl:sendfile/5 to ranch_transport:sendfile/6 as fallback for custom transports.
  • Rewrote tests without proper, with tests ensuring behaviour identical between ranch_ssl to ranch_tcp.
  • Add default chunk_size to ranch_tcp:sendfile/2 (same as ranch_ssl:senfile/2).
  • Added information to guide.
  • Completed inline docs.
@fishcakez

This comment has been minimized.

Contributor

fishcakez commented Jun 20, 2013

  • Updated for R16B01.
@essen

This comment has been minimized.

Member

essen commented Jun 29, 2013

OK this is a big one and I can already feel the need for food, so I'll keep a tab open for later.

@@ -118,6 +118,29 @@ end.
You can easily integrate active sockets with existing Erlang code as all
you really need is just a few more clauses when receiving messages.
It is also possible to send a whole file:

This comment has been minimized.

@essen

essen Jul 5, 2013

Member

Should be in its own section, like "Sending files" to make it more obvious.

@essen

This comment has been minimized.

Member

essen commented Jul 5, 2013

Please generate the binary file during tests (once at the beginning of the test suite is fine) instead of including it.

@@ -192,27 +194,34 @@ send(Socket, Packet) ->
%% through SSL will be much slower in comparison.
%%
%% @see file:sendfile/2

This comment has been minimized.

@essen

essen Jul 5, 2013

Member

You can remove the doc comment here and sendfile/4 and only keep it on sendfile/5. equiv is enough on the other two.

-spec sendfile(ssl:sslsocket(), file:name_all())
-> ranch_transport:sendfile_return().
sendfile(Socket, Filename) ->
sendfile(Socket, Filename, 0, 0).

This comment has been minimized.

@essen

essen Jul 5, 2013

Member

Why use sendfile/4 when you can use sendfile/5 directly.

-> {ok, non_neg_integer()} | {error, atom()}.
sendfile(Socket, Filename) ->
try file:sendfile(Filename, Socket) of

This comment has been minimized.

@essen

essen Jul 5, 2013

Member

For this function in particular it's probably a better idea to call file:sendfile/2 directly and avoid the overhead.

This comment has been minimized.

@essen

essen Jul 5, 2013

Member

On the other hand same remark for the doc comment here, equiv is enough.

This comment has been minimized.

@fishcakez

fishcakez Jul 5, 2013

Contributor

If we call file:sendfile/2 directly we don't close the file inside a try/catch, are vulnerable to #39 and have default of 20MB chunk size. I am happy to call file:sendfile/2 directly but file:sendfile/2 calls file:sendfile/5 anyway so I'm unsure there is a saving here once I resolve the later two of the three I mentioned. As file:sendfile/2 does not ensure the file is closed shall I remove the try/catches for opening?

Ps: Thanks for thorough review, must have taken some time.

This comment has been minimized.

@essen

essen Jul 5, 2013

Member

Then disregard.

Error
end;
sendfile(Socket, RawFile, Offset, Bytes, Opts) ->
Opts2 = Opts ++ [{chunk_size, 16#1FFF}],

This comment has been minimized.

@essen

essen Jul 5, 2013

Member

What happens if chunk_size was already defined? Is it guaranteed that the first one will always be chosen, if that's what currently happen?

This comment has been minimized.

@fishcakez

fishcakez Jul 5, 2013

Contributor

It is not documented but file:sendfile/5 uses proplists:get_value/3. I can use lists:keymember/3 and only add if not present if you like.

This comment has been minimized.

@essen

essen Jul 5, 2013

Member

Sounds safer.

This comment has been minimized.

@essen

essen Jul 5, 2013

Member

Actually just match on [] since there's no other option yet.

try sendfile(Socket, RawFile, Offset, Bytes, Opts) of
Result -> Result
after
%% Ensure file is closed.

This comment has been minimized.

@essen

essen Jul 5, 2013

Member

I think we figured it out without the comment.

-type sendfile_opts() :: [{chunk_size, non_neg_integer()}].
-export_type([sendfile_opts/0]).
-type sendfile_return() :: {ok, non_neg_integer()} | {error, atom()}.
-export_type([sendfile_return/0]).

This comment has been minimized.

@essen

essen Jul 5, 2013

Member

I don't see the point of exporting this type.

Also they all should be after the more important types like socket.

@@ -55,6 +62,19 @@
%% Send data on a socket.
-callback send(socket(), iodata()) -> ok | {error, atom()}.
%% Send a file on a socket.

This comment has been minimized.

@essen

essen Jul 5, 2013

Member

to?

This comment has been minimized.

@fishcakez

fishcakez Jul 5, 2013

Contributor

The phrase "on a socket" is used everywhere, including in the current sendfile/2.

This comment has been minimized.

@essen

essen Jul 5, 2013

Member

OK I guess.

%% Send part of a file on a socket.
-callback sendfile(socket(), file:name() | file:fd(), non_neg_integer(),
non_neg_integer(), list())

This comment has been minimized.

@essen

essen Jul 5, 2013

Member

list()? Sounds like sendfile_opts().

sendfile_after(RawFile, close) ->
%% We opened file so must close it.
ok = file:close(RawFile);
sendfile_after(RawFile, AfterPos) ->

This comment has been minimized.

@essen

essen Jul 5, 2013

Member

I don't see this clause ever reached.

This comment has been minimized.

@fishcakez

fishcakez Jul 5, 2013

Contributor

I will investigate this.

This comment has been minimized.

@fishcakez

fishcakez Jul 5, 2013

Contributor

The function is called inside ranch_transport:sendfile/8 in the after clause of the try. In the tests I check that {ok, Initial} = file:position(RawFile, {cur, 0}) in the rawfile_range_{medium/large/small}/1 tests. This means that the file position has been reset by sendfile_after/2.

This comment has been minimized.

@essen

essen Jul 5, 2013

Member

Both sendfile_before and sendfile_after are confusing to me. First there's sendfile/7 with two clauses, one where we open the file and one with fd, where we read the position. Then there's sendfile_before that's checking that position and only moving if we're not at the expected position. Then things happen. Then sendfile_after may close the file or restore the position.

I'd remove sendfile/8 and do the get/set position in the clauses directly, and the closing of the file in the clause too. Don't keep unneeded state around if we can make it obvious. That does mean two try..after but they're not exactly big, the code will probably still be smaller, and for sure simpler to follow.

This comment has been minimized.

@fishcakez

fishcakez Jul 5, 2013

Contributor

Yes much better now. I can also removed the try..after from the RawFile version because position is undefined on error.

@@ -0,0 +1,324 @@
%% Copyright (c) 2011-2012, Loïc Hoguin <essen@ninenines.eu>

This comment has been minimized.

@essen

essen Jul 5, 2013

Member

I didn't touch the file man.

@essen

This comment has been minimized.

Member

essen commented Jul 5, 2013

I'll trust you on the tests, however can we also test a large file, a few megabytes?

@essen

This comment has been minimized.

Member

essen commented Jul 5, 2013

Review done for now!

@fishcakez

This comment has been minimized.

Contributor

fishcakez commented Jul 6, 2013

I believe I have made all changes except that the test suite creates (and deletes after) a 20MB file and uses that for every test - instead of adding a larger file test.

{ok, SentBytes} = Transport:sendfile(Socket, Filename, Offset, Bytes, Opts).
```
To improves efficiency when sending multiple parts of the same file it is also

This comment has been minimized.

@essen

essen Jul 12, 2013

Member

improve

```
To improves efficiency when sending multiple parts of the same file it is also
possible to use a `file:fd()` - a file opened in `raw` (and `read`) mode. The

This comment has been minimized.

@essen

essen Jul 12, 2013

Member

Can probably be said better. Like "it is also possible to use a file descriptor opened in raw mode." No need to get too much into details everyone will get that it's a file:fd() and that it must be open for reading!

@@ -186,33 +188,32 @@ recv(Socket, Length, Timeout) ->
send(Socket, Packet) ->
ssl:send(Socket, Packet).
%% @doc Send a file on a socket.
%% @equiv sendfile(Socket, Filename, 0, 0, []).

This comment has been minimized.

@essen

essen Jul 12, 2013

Member

No dot there please.

sendfile(Socket, Filename) ->
sendfile(Socket, Filename, 0, 0, []).
%% @equiv sendfile(Socket, File, Offset, Bytes, []).

This comment has been minimized.

@essen
@@ -110,21 +112,54 @@ recv(Socket, Length, Timeout) ->
send(Socket, Packet) ->
gen_tcp:send(Socket, Packet).
%% @doc Send a file on a socket.
%% @equiv sendfile(Socket, File, Offset, Bytes, []).

This comment has been minimized.

@essen
sendfile(Socket, Filename) ->
sendfile(Socket, Filename, 0, 0, []).
%% @equiv sendfile(Socket, File, Offset, Bytes, []).

This comment has been minimized.

@essen
[{chunk_size, 16#1FFF}];
_ ->
Opts
end,

This comment has been minimized.

@essen

essen Jul 12, 2013

Member

Think you booped the indentation there.

@essen

This comment has been minimized.

Member

essen commented Aug 10, 2013

Hi! :)

@fishcakez

This comment has been minimized.

Contributor

fishcakez commented Aug 15, 2013

Hi :). OOps. Will have a look at doing these changes tomorrow/improvements.

Add Transport:sendfile/4,/5
Adds offset based sendfile to transports. Same behaviour as
file:sendfile/4,/5 except socket and file arguments are reversed and
either a raw file or a filename can be used.

sendfile/2,/4,/5 now compulsory callbacks in ranch_transport.

ranch_tcp:sendfile/2 now defaults to a chunk_size of 8191 - the default
for ranch_ssl:sendfile/2. The same default is used for both
ranch_tcp:sendfile/4,5 and ranch_ssl:sendfile/4,5.
@fishcakez

This comment has been minimized.

Contributor

fishcakez commented Aug 16, 2013

Updated.

Edit: I didn't do improvements suggested by previous comment. I thought I had to improve something but it was just a confused memory over the improves typo.

@essen essen merged commit ca68178 into ninenines:master Aug 27, 2013

@essen

This comment has been minimized.

Member

essen commented Aug 27, 2013

Merged! YARLY. Awesome work, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment