[#2209] Allow to cancel non-flushed writes #2214
Conversation
|
@trustin let me know what you think. I'm not sure yet if we really want to support cancel writes at all. |
|
Beside this. i think we also need to take care of handle cancelation for other io operations like close. Somethink like call setUncancellable() and check the return value before really do the operation |
| final int mask = buffer.length - 1; | ||
| while (i != unflushed && buffer[i].msg != null) { | ||
| Entry entry = buffer[i]; | ||
| if (!entry.promise.setUncancellable() && entry.promise.isCancelled()) { |
There was a problem hiding this comment.
Can't you do this in current(..) and nioBuffers()?
There was a problem hiding this comment.
I thought it make more sense here as it is the point where we flush.
Am 08.02.2014 um 01:31 schrieb Trustin Lee notifications@github.com:
In transport/src/main/java/io/netty/channel/ChannelOutboundBuffer.java:
@@ -163,6 +164,19 @@ private void addCapacity() {
void addFlush() { unflushed = tail;
int i = flushed;final int mask = buffer.length - 1;while (i != unflushed && buffer[i].msg != null) {Entry entry = buffer[i];if (!entry.promise.setUncancellable() && entry.promise.isCancelled()) {—
Reply to this email directly or view it on GitHub.
There was a problem hiding this comment.
On the other hand maybe do it in current and nioBuffers would ne not a bad idea
Am 08.02.2014 um 01:31 schrieb Trustin Lee notifications@github.com:
In transport/src/main/java/io/netty/channel/ChannelOutboundBuffer.java:
@@ -163,6 +164,19 @@ private void addCapacity() {
void addFlush() { unflushed = tail;
int i = flushed;final int mask = buffer.length - 1;while (i != unflushed && buffer[i].msg != null) {Entry entry = buffer[i];if (!entry.promise.setUncancellable() && entry.promise.isCancelled()) {—
Reply to this email directly or view it on GitHub.
|
@trustin please check again... running benchmark now |
|
No speed difference |
| @@ -797,6 +814,13 @@ protected static void checkEOF(FileRegion region) throws IOException { | |||
| } | |||
| } | |||
|
|
|||
| protected static boolean checkCancelled(ChannelPromise promise) { | |||
| if (!promise.setUncancellable() && promise.isCancelled()) { | |||
There was a problem hiding this comment.
If setUncancellable() returns false, it means the promise has been cancelled already. i.e. promise.isCancelled() is redundant.
There was a problem hiding this comment.
Actually, it only tells us isDone() is true. However, I'm not sure it's a good idea to continue performing the operation when the promise is done. We should probably log a warning and then discard the request probably?
- Inspired by #2214 - It actually reduces the chance of trying to marking a cancelled promise as success again, which raises an IllegalStateException.
- Inspired by #2214 - It actually reduces the chance of trying to marking a cancelled promise as success again, which raises an IllegalStateException.
- Inspired by #2214 by @normanmaurer - Call setUncancellable() before performing an outbound operation - Add safeSetSuccess/Failure() and use them wherever
- Inspired by #2214 by @normanmaurer - Call setUncancellable() before performing an outbound operation - Add safeSetSuccess/Failure() and use them wherever
|
@trustin please review again |
|
LGTM. Please merge. |
|
@trustin cherry-picked into 4.0 and master |
Proposed fix for #2209