andreacavalli/netty5
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Cleanup : validatePromise ranamed to isNotValidPromise and added more tests for corner cases.

Browse Source 
Motivation:

Result of validatePromise() is always inverted with if (!validatePromise()).

Modification:

validatePromise() renamed to isNotValidPromise() and now returns inverted state so you don't need to invert state in conditions. Also name is now more meaningful according to returned result.
Added more tests for validatePromise corner cases with Exceptions.

Result:

Code easier to read. No need in inverted result.
This commit is contained in:
Dmitriy Dumanskiy 2017-02-09 20:10:49 +02:00 committed by Norman Maurer
parent c95517f759
commit 64838f1505
2 changed files with 54 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
transport/src/main/java/io/netty/channel/AbstractChannelHandlerContext.java
View File

@ -475,7 +475,7 @@ abstract class AbstractChannelHandlerContext extends DefaultAttributeMap
if (localAddress == null) { if (localAddress == null) {
throw new NullPointerException("localAddress"); throw new NullPointerException("localAddress");
} }
if (!validatePromise(promise, false)) { if (isNotValidPromise(promise, false)) {
// cancelled // cancelled
return promise; return promise;
} }
@ -519,7 +519,7 @@ abstract class AbstractChannelHandlerContext extends DefaultAttributeMap
if (remoteAddress == null) { if (remoteAddress == null) {
throw new NullPointerException("remoteAddress"); throw new NullPointerException("remoteAddress");
} }
if (!validatePromise(promise, false)) { if (isNotValidPromise(promise, false)) {
// cancelled // cancelled
return promise; return promise;
} }
@ -553,7 +553,7 @@ abstract class AbstractChannelHandlerContext extends DefaultAttributeMap
@Override @Override
public ChannelFuture disconnect(final ChannelPromise promise) { public ChannelFuture disconnect(final ChannelPromise promise) {
if (!validatePromise(promise, false)) { if (isNotValidPromise(promise, false)) {
// cancelled // cancelled
return promise; return promise;
} }
@ -597,7 +597,7 @@ abstract class AbstractChannelHandlerContext extends DefaultAttributeMap
@Override @Override
public ChannelFuture close(final ChannelPromise promise) { public ChannelFuture close(final ChannelPromise promise) {
if (!validatePromise(promise, false)) { if (isNotValidPromise(promise, false)) {
// cancelled // cancelled
return promise; return promise;
} }
@ -632,7 +632,7 @@ abstract class AbstractChannelHandlerContext extends DefaultAttributeMap
@Override @Override
public ChannelFuture deregister(final ChannelPromise promise) { public ChannelFuture deregister(final ChannelPromise promise) {
if (!validatePromise(promise, false)) { if (isNotValidPromise(promise, false)) {
// cancelled // cancelled
return promise; return promise;
} }
@ -711,7 +711,7 @@ abstract class AbstractChannelHandlerContext extends DefaultAttributeMap
} }
try { try {
if (!validatePromise(promise, true)) { if (isNotValidPromise(promise, true)) {
ReferenceCountUtil.release(msg); ReferenceCountUtil.release(msg);
// cancelled // cancelled
return promise; return promise;
@ -785,7 +785,7 @@ abstract class AbstractChannelHandlerContext extends DefaultAttributeMap
throw new NullPointerException("msg"); throw new NullPointerException("msg");
} }
if (!validatePromise(promise, true)) { if (isNotValidPromise(promise, true)) {
ReferenceCountUtil.release(msg); ReferenceCountUtil.release(msg);
// cancelled // cancelled
return promise; return promise;
@ -894,7 +894,7 @@ abstract class AbstractChannelHandlerContext extends DefaultAttributeMap
return new FailedChannelFuture(channel(), executor(), cause); return new FailedChannelFuture(channel(), executor(), cause);
} }
private boolean validatePromise(ChannelPromise promise, boolean allowVoidPromise) { private boolean isNotValidPromise(ChannelPromise promise, boolean allowVoidPromise) {
if (promise == null) { if (promise == null) {
throw new NullPointerException("promise"); throw new NullPointerException("promise");
} }
@ -905,7 +905,7 @@ abstract class AbstractChannelHandlerContext extends DefaultAttributeMap
// //
// See https://github.com/netty/netty/issues/2349 // See https://github.com/netty/netty/issues/2349
if (promise.isCancelled()) { if (promise.isCancelled()) {
return false; return true;
} }
throw new IllegalArgumentException("promise already done: " + promise); throw new IllegalArgumentException("promise already done: " + promise);
} }
@ -916,7 +916,7 @@ abstract class AbstractChannelHandlerContext extends DefaultAttributeMap
} }
if (promise.getClass() == DefaultChannelPromise.class) { if (promise.getClass() == DefaultChannelPromise.class) {
return true; return false;
} }
if (!allowVoidPromise && promise instanceof VoidChannelPromise) { if (!allowVoidPromise && promise instanceof VoidChannelPromise) {
@ -928,7 +928,7 @@ abstract class AbstractChannelHandlerContext extends DefaultAttributeMap
throw new IllegalArgumentException( throw new IllegalArgumentException(
StringUtil.simpleClassName(AbstractChannel.CloseFuture.class) + " not allowed in a pipeline"); StringUtil.simpleClassName(AbstractChannel.CloseFuture.class) + " not allowed in a pipeline");
} }
return true; return false;
} }
private AbstractChannelHandlerContext findContextInbound() { private AbstractChannelHandlerContext findContextInbound() {

43
transport/src/test/java/io/netty/channel/DefaultChannelPipelineTest.java
View File

@ -504,6 +504,49 @@ public class DefaultChannelPipelineTest {
assertTrue(future.isCancelled()); assertTrue(future.isCancelled());
} }
@Test(expected = IllegalArgumentException.class)
public void testWrongPromiseChannel() throws Exception {
ChannelPipeline pipeline = new LocalChannel().pipeline();
group.register(pipeline.channel()).sync();
ChannelPipeline pipeline2 = new LocalChannel().pipeline();
group.register(pipeline2.channel()).sync();
try {
ChannelPromise promise2 = pipeline2.channel().newPromise();
pipeline.close(promise2);
} finally {
pipeline.close();
pipeline2.close();
}
}
@Test(expected = IllegalArgumentException.class)
public void testUnexpectedVoidChannelPromise() throws Exception {
ChannelPipeline pipeline = new LocalChannel().pipeline();
group.register(pipeline.channel()).sync();
try {
ChannelPromise promise = new VoidChannelPromise(pipeline.channel(), false);
pipeline.close(promise);
} finally {
pipeline.close();
}
}
@Test(expected = IllegalArgumentException.class)
public void testUnexpectedVoidChannelPromiseCloseFuture() throws Exception {
ChannelPipeline pipeline = new LocalChannel().pipeline();
group.register(pipeline.channel()).sync();
try {
ChannelPromise promise = (ChannelPromise) pipeline.channel().closeFuture();
pipeline.close(promise);
} finally {
pipeline.close();
}
}
@Test @Test
public void testCancelDeregister() throws Exception { public void testCancelDeregister() throws Exception {
ChannelPipeline pipeline = new LocalChannel().pipeline(); ChannelPipeline pipeline = new LocalChannel().pipeline();