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.

Sign up for GitHub

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

Jump to bottom

Fail better wrt sendfile over SSL socket #17

Closed
wants to merge 1 commit into from
Closed

Fail better wrt sendfile over SSL socket #17

wants to merge 1 commit into from

Conversation

yurrriq
Copy link
Member

@yurrriq yurrriq commented Nov 9, 2016

Feedback wanted.

@yurrriq
Update elli_http and elli_ssl_tests
b5d231c 
Add clause to do_send_file/4 to fail on SSL sockets. Otherwise,
elli_tcp:sendfile/5 throws, but the client still gets an empty 200
response, due the the way the other do_send_file/4 clause sends the
headers before calling elli_tcp:sendfile/5.

I've pulled this out into a branch/PR, because I think this solution is
contentious but highlights an important issue.
@deadtrickster
Copy link
Member

Build is broken :-(

@deadtrickster
Copy link
Member

How to send file to SSL client then?
I think it's better to fallback to manual implementation.
BTW elli_tcp already throws when sendfile used with ssl:

elli/src/elli_tcp.erl

Line 76 in b5d231c

sendfile(_Fd, {ssl, _}, _Offset, _Length, _Opts) ->
. We need to rewrite this {ssl, } clause to send bytes manually.

@yurrriq
Copy link
Member Author

yurrriq commented Nov 9, 2016

It throws but still responds with an empty 200 response.

@yurrriq
Copy link
Member Author

yurrriq commented Nov 9, 2016

+1

@yurrriq yurrriq mentioned this pull request Nov 9, 2016
Closed
@yurrriq
Copy link
Member Author

yurrriq commented Nov 9, 2016

Closing in favor of a better solution to #18.

@yurrriq yurrriq closed this Nov 9, 2016
@yurrriq yurrriq deleted the feature/sendfile-hacks branch March 13, 2018 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@yurrriq @deadtrickster