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

Add handling of offset and length to NettyOutbound#sendFileChunked #320

Closed
wants to merge 2 commits into from

Conversation

lhotari
Copy link
Contributor

@lhotari lhotari commented Apr 12, 2018

  • The position and count parameters on reactor.ipc.netty.NettyOutbound#sendFileChunked method calls were ignored.
  • sendFile tests are now refactored so that same tests are used for both http and https
    • new tests for SSL reveal problem with sendFileAsync4096/sendFileAsync1024 (ignored for now)

@codecov-io
Copy link

codecov-io commented Apr 12, 2018

Codecov Report

Merging #320 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #320      +/-   ##
============================================
+ Coverage     68.28%   68.37%   +0.09%     
- Complexity     1022     1024       +2     
============================================
  Files            73       73              
  Lines          4310     4310              
  Branches        615      615              
============================================
+ Hits           2943     2947       +4     
+ Misses          996      993       -3     
+ Partials        371      370       -1
Impacted Files Coverage Δ Complexity Δ
...etty/channel/data/AbstractFileChunkedStrategy.java 69.56% <ø> (ø) 6 <0> (ø) ⬇️
src/main/java/reactor/ipc/netty/NettyOutbound.java 84.74% <100%> (ø) 27 <1> (ø) ⬇️
...in/java/reactor/ipc/netty/http/HttpOperations.java 68.83% <100%> (ø) 22 <0> (ø) ⬇️
src/main/java/reactor/ipc/netty/FutureMono.java 82% <0%> (-4%) 5% <0%> (-1%)
.../ipc/netty/channel/PooledClientContextHandler.java 64.7% <0%> (-3.37%) 26% <0%> (-1%)
...or/ipc/netty/http/server/HttpServerOperations.java 71.05% <0%> (+0.52%) 56% <0%> (+1%) ⬆️
...or/ipc/netty/http/client/HttpClientOperations.java 57.1% <0%> (+0.52%) 79% <0%> (ø) ⬇️
...a/reactor/ipc/netty/channel/ChannelOperations.java 83.94% <0%> (+1.45%) 48% <0%> (+1%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bcdbd9c...1820d20. Read the comment docs.

@violetagg violetagg added this to the 0.7.7.RELEASE milestone Apr 12, 2018
@violetagg violetagg added the type/bug A general bug label Apr 12, 2018
@violetagg violetagg self-assigned this Apr 12, 2018
protected void customizeClientOptions(HttpClientOptions.Builder options) {
try {
options.sslContext(SslContextBuilder
.forServer(ssc.certificate(), ssc.privateKey()).build());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you create SslContext for server when customising the client options?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, fixed.

@Override
protected void customizeServerOptions(HttpServerOptions.Builder options) {
try {
options.sslContext(SslContextBuilder.forClient()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you create SslContext for client when customising the server options?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, fixed.

- new tests for SSL reveal problem with sendFileAsync4096/sendFileAsync1024
  when SSL is used
@violetagg
Copy link
Member

Thanks
The commits were applied 6e89363, c886024
I created a new issue for the failing test #321

@violetagg violetagg closed this Apr 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants