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 #289 Register pool-releasing close listener only if acquired #290

Merged
merged 5 commits into from
Feb 19, 2018

Conversation

simonbasle
Copy link
Member

Instead of setting up the close listener in the ChannelPoolHandler
channelCreated method, set it up in the PooledClientContextHandler.

This should prevent the listener to be invoked after a FixedChannelPool
has determined the channel was not healthy and has already decremented
its internal acquire counter (which would lead to a second decrement of
said counter and an assertion error).

only set the channel closeFuture to release to the pool once we've made
sure that we acquired a Channel.
@codecov-io
Copy link

codecov-io commented Feb 19, 2018

Codecov Report

Merging #290 into master will increase coverage by 0.68%.
The diff coverage is 87.5%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #290      +/-   ##
============================================
+ Coverage     67.78%   68.46%   +0.68%     
- Complexity     1015     1026      +11     
============================================
  Files            73       73              
  Lines          4287     4291       +4     
  Branches        611      613       +2     
============================================
+ Hits           2906     2938      +32     
+ Misses         1011      989      -22     
+ Partials        370      364       -6
Impacted Files Coverage Δ Complexity Δ
.../ipc/netty/channel/PooledClientContextHandler.java 70.58% <ø> (+5.58%) 29 <0> (+2) ⬆️
...ctor/ipc/netty/resources/DefaultPoolResources.java 75.86% <87.5%> (+2.69%) 8 <0> (ø) ⬇️
...or/ipc/netty/channel/ChannelOperationsHandler.java 63.63% <0%> (+0.93%) 58% <0%> (+2%) ⬆️
...a/reactor/ipc/netty/channel/ChannelOperations.java 84.96% <0%> (+1.5%) 49% <0%> (+1%) ⬆️
...or/ipc/netty/http/client/HttpClientOperations.java 56.3% <0%> (+1.6%) 74% <0%> (+2%) ⬆️
src/main/java/reactor/ipc/netty/tcp/TcpClient.java 90.56% <0%> (+1.88%) 20% <0%> (+1%) ⬆️
...java/reactor/ipc/netty/channel/ContextHandler.java 76.47% <0%> (+1.96%) 28% <0%> (+1%) ⬆️
...tor/ipc/netty/channel/CloseableContextHandler.java 57.89% <0%> (+5.26%) 9% <0%> (+1%) ⬆️
...va/reactor/ipc/netty/channel/AbortedException.java 81.81% <0%> (+27.27%) 7% <0%> (+1%) ⬆️

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 db8a72f...ff00d41. Read the comment docs.

@simonbasle
Copy link
Member Author

please heavily challenge the relevance of this fix. adding a little background on the issue itself.

@violetagg violetagg added this to the 0.7.5.RELEASE milestone Feb 19, 2018
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.

Review indicates the close listener is being invoked as many times as it has been acquired. Looking into this

@smaldini smaldini merged commit 39cbb8c into master Feb 19, 2018
@smaldini smaldini deleted the 289-channelPoolDoubleRelease branch February 19, 2018 22:02
@smaldini smaldini added the type/bug A general bug label Feb 19, 2018
public void fixedPoolTwoAcquire()
throws ExecutionException, InterruptedException, IOException {
final ScheduledExecutorService service = Executors.newScheduledThreadPool(2);
int echoServerPort = SocketUtils.findAvailableTcpPort();

Choose a reason for hiding this comment

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

FYI, this is not entirely reliable and can cause problems on CI servers, if some other process grabs the socket. We've seen this happen once in a while, causing bogus CI build failures.

In my projects, where possible, we've changed tests to listen on port 0 (so the OS selects the port) and then query the test server for which port was actually selected.

Not critical but worth consideration.

Copy link
Member Author

Choose a reason for hiding this comment

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

hard to do as the first part of the test checks what happens when the host is initially unreachable. crossing fingers for CI :(

artembilan added a commit to artembilan/spring-integration that referenced this pull request Feb 20, 2018
* Fix `StompServerIntegrationTests` for disabling connection pool to
workaround the regression in the Reactor-Netty-0.7.4

Related to reactor/reactor-netty#290
garyrussell pushed a commit to spring-projects/spring-integration that referenced this pull request Feb 20, 2018
* Fix `StompServerIntegrationTests` for disabling connection pool to
workaround the regression in the Reactor-Netty-0.7.4

Related to reactor/reactor-netty#290
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.

5 participants