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

fix #495 Ensure all children channels are closed when invoking DisposableServer#dispose #497

Closed
wants to merge 1 commit into from

Conversation

violetagg
Copy link
Member

No description provided.

@violetagg violetagg requested a review from smaldini November 6, 2018 14:29
@violetagg violetagg added this to the 0.8.3.RELEASE milestone Nov 6, 2018
@codecov-io
Copy link

codecov-io commented Nov 6, 2018

Codecov Report

Merging #497 into master will decrease coverage by 0.25%.
The diff coverage is 94.11%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #497      +/-   ##
============================================
- Coverage     63.91%   63.65%   -0.26%     
+ Complexity     1332     1308      -24     
============================================
  Files           126      125       -1     
  Lines          6354     6271      -83     
  Branches        846      836      -10     
============================================
- Hits           4061     3992      -69     
- Misses         1841     1842       +1     
+ Partials        452      437      -15
Impacted Files Coverage Δ Complexity Δ
...ain/java/reactor/netty/http/server/HttpServer.java 68.05% <ø> (ø) 30 <0> (ø) ⬇️
src/main/java/reactor/netty/tcp/TcpServer.java 51.38% <ø> (+2.77%) 23 <0> (+2) ⬆️
src/main/java/reactor/netty/tcp/TcpServerBind.java 87.23% <100%> (+0.27%) 11 <0> (ø) ⬇️
...java/reactor/netty/http/server/HttpServerBind.java 37.76% <85.71%> (-0.17%) 21 <9> (+1)
src/main/java/reactor/netty/tcp/ProxyProvider.java 47.22% <0%> (-17.83%) 14% <0%> (-7%)
...ctor/netty/resources/PooledConnectionProvider.java 72.51% <0%> (-5.65%) 14% <0%> (-6%)
.../reactor/netty/http/client/WebsocketFinalizer.java 70% <0%> (-2.73%) 5% <0%> (-1%)
...reactor/netty/http/client/HttpClientFinalizer.java 79.48% <0%> (-2.57%) 20% <0%> (ø)
...r/netty/http/server/WebsocketServerOperations.java 62.12% <0%> (-2.52%) 16% <0%> (-3%)
src/main/java/reactor/netty/ByteBufMono.java 36.58% <0%> (-2.44%) 7% <0%> (-1%)
... and 27 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 db96bed...8bda36d. Read the comment docs.

…ableServer#dispose

When using the server with the default Tcp/HttpResources, when the server is disposed,
they also will be disposed thus the children channels will be closed also.
@violetagg violetagg modified the milestones: 0.8.3.RELEASE, 0.8.x Backlog, 0.8.4.RELEASE Nov 20, 2018
@violetagg violetagg modified the milestones: 0.8.4.RELEASE, 0.8.5.RELEASE Jan 8, 2019
Copy link
Contributor

@smaldini smaldini left a comment

Choose a reason for hiding this comment

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

The following feels a bit hacky and prone to errors for multiple subscribe (even if that is niche case, its still a possibility for the user).

s -> {  disposableBind.server = s; 
//... 
}```

I wonder if we just need to document this or add a separate explicit option to the server creation like `cleanEventLoopsOnDispose(boolean)` with the documentation referring to the shutdown of underlying channels.

@smaldini smaldini modified the milestones: 0.8.5.RELEASE, 0.8.x Backlog, 0.9.x Backlog Feb 7, 2019
@violetagg
Copy link
Member Author

Closing this in favour of the fix for #586 in particular channelGroup

@violetagg violetagg closed this Feb 18, 2019
@violetagg violetagg deleted the issue-495 branch February 18, 2019 18:35
@violetagg violetagg removed this from the 0.9.x Backlog milestone Mar 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants