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

Do not flush cancelled writes. #2209

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@xfrag

xfrag commented Feb 7, 2014

My experience with Netty is pretty limited, so I'm not sure if this is an issue or not. The fact is that the following lines will result in an actual channel write when I expected none:

        ChannelHandlerContext ctx = ...
        ByteBuf out = ...
        ctx.write(out).cancel(false);
        ctx.flush();

A thorough example can be found here. Were there any plans to support such use cases in Netty? Looks like my changes don't break anything, the project compiles with all tests passed.

Thanks

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Feb 7, 2014

Build result for #2209 at 5f942ed: Success

ghost commented Feb 7, 2014

Build result for #2209 at 5f942ed: Success

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Feb 7, 2014

Member

@xfrag this is a bug... Your fix is not 100 % correct, let me adjust it a bit and push a fix.

Thanks for reporting

Member

normanmaurer commented Feb 7, 2014

@xfrag this is a bug... Your fix is not 100 % correct, let me adjust it a bit and push a fix.

Thanks for reporting

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Feb 7, 2014

Member

@trustin I wonder if we want to support this or just make sure to call setUncancellable(..) on the future ?

Member

normanmaurer commented Feb 7, 2014

@trustin I wonder if we want to support this or just make sure to call setUncancellable(..) on the future ?

@xfrag

This comment has been minimized.

Show comment
Hide comment
@xfrag

xfrag Feb 7, 2014

@normanmaurer you're welcome. Will wait for your fix.

Since you're considering to prohibit the cancellation of a write, let me outline my use case to justify why I think it should be allowed:

The target is to serve a live bitstream, live meaning that the stream is generated on the fly and a client may connect and start receiving it at any point. In my particular use case it happens to be an audio stream. A server component manages the stream server-side, and this component may be 'up' or 'down'. When 'up' clients may connect, when 'down' clients connections are rejected. When a connection is accepted, the component immediately starts writing stream data to the client's channel.

Once a connection request is received the following steps take place:

  1. Check the state of the stream component.
  2. Component is 'down': go to 7).
  3. Component is 'up': prepare and write a confirmation response.
  4. Connect the channel with the component.
  5. Connection accepted: (component is still 'up') - the confirmation will be flushed along with the
    streamed data; go to END.
  6. Connection rejected: (component went 'down') - cancel the confirmation write.
  7. Write and flush a rejection response, close the connection.
    END

Now since the stream component does not switch states so frequently, one could write/flush a confirmation in 3) and then simply close the connection in the rare cases where 6) occurs. Yet if it is possible to cancel the confirmation and write/flush a rejection response instead, why not? I mean it makes perfect sense to me to be able to cancel a write when it is not yet flushed.

Sorry for this lengthy comment. It is only a week or so that I started using Netty, so if there is a simpler way to think of the above use case, hints are welcome!

xfrag commented Feb 7, 2014

@normanmaurer you're welcome. Will wait for your fix.

Since you're considering to prohibit the cancellation of a write, let me outline my use case to justify why I think it should be allowed:

The target is to serve a live bitstream, live meaning that the stream is generated on the fly and a client may connect and start receiving it at any point. In my particular use case it happens to be an audio stream. A server component manages the stream server-side, and this component may be 'up' or 'down'. When 'up' clients may connect, when 'down' clients connections are rejected. When a connection is accepted, the component immediately starts writing stream data to the client's channel.

Once a connection request is received the following steps take place:

  1. Check the state of the stream component.
  2. Component is 'down': go to 7).
  3. Component is 'up': prepare and write a confirmation response.
  4. Connect the channel with the component.
  5. Connection accepted: (component is still 'up') - the confirmation will be flushed along with the
    streamed data; go to END.
  6. Connection rejected: (component went 'down') - cancel the confirmation write.
  7. Write and flush a rejection response, close the connection.
    END

Now since the stream component does not switch states so frequently, one could write/flush a confirmation in 3) and then simply close the connection in the rare cases where 6) occurs. Yet if it is possible to cancel the confirmation and write/flush a rejection response instead, why not? I mean it makes perfect sense to me to be able to cancel a write when it is not yet flushed.

Sorry for this lengthy comment. It is only a week or so that I started using Netty, so if there is a simpler way to think of the above use case, hints are welcome!

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Feb 7, 2014

Member

@xfrag I think I see why you want to cancel a write even if I think you could just also make it work without canceling. It is more a question if we want to pay the extra price to support it. As this will maybe slow down things (as we need to check for isCancelled() etc all the time.,

Member

normanmaurer commented Feb 7, 2014

@xfrag I think I see why you want to cancel a write even if I think you could just also make it work without canceling. It is more a question if we want to pay the extra price to support it. As this will maybe slow down things (as we need to check for isCancelled() etc all the time.,

@xfrag

This comment has been minimized.

Show comment
Hide comment
@xfrag

xfrag Feb 8, 2014

@normanmaurer I get your point and I agree, most of the times there is no need to cancel a write so the potential overhead of checking would be a waste of resources.

But then, why should one write and then flush? I don't have a clear understanding of the reason for having to break the write operation in two steps, when the first step doesn't have a meaning without the second. The public API could simply expose a write() method that would be internally handled like writeAndFlush, and hide the flush() method. Is this backwards-compatibility related?

Another thought: could the ability to cancel writes be made optional, in order to reduce the checking overhead? This way, the user will have to enable cancellation support for the period of time during which a cancellation might occur, then disable and return back to the not-cancel-able mode. Checking a boolean state will practically impose no overhead. I will try to implement this and if I get into anything actually functional, will issue another pull request.

xfrag commented Feb 8, 2014

@normanmaurer I get your point and I agree, most of the times there is no need to cancel a write so the potential overhead of checking would be a waste of resources.

But then, why should one write and then flush? I don't have a clear understanding of the reason for having to break the write operation in two steps, when the first step doesn't have a meaning without the second. The public API could simply expose a write() method that would be internally handled like writeAndFlush, and hide the flush() method. Is this backwards-compatibility related?

Another thought: could the ability to cancel writes be made optional, in order to reduce the checking overhead? This way, the user will have to enable cancellation support for the period of time during which a cancellation might occur, then disable and return back to the not-cancel-able mode. Checking a boolean state will practically impose no overhead. I will try to implement this and if I get into anything actually functional, will issue another pull request.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Feb 8, 2014

Member

Having write and flush seperated allows to easily implement pipelining. So a user call multiple times write and then flush to try to write everything to the socket with one syscall

Am 08.02.2014 um 04:01 schrieb Christos Fragoulides notifications@github.com:

@normanmaurer I get your point and I agree, most of the times there is no need to cancel a write so the potential overhead of checking would be a waste of resources.

But then, why should one write and then flush? I don't have a clear understanding of the reason for having to break the write operation in two steps, when the first step doesn't have a meaning without the second. The public API could simply expose a write() method that would be internally handled like writeAndFlush, and hide the flush() method. Is this backwards-compatibility related?

Another thought: could the ability to cancel writes be made optional, in order to reduce the checking overhead? This way, the user will have to enable cancellation support for the period of time during which a cancellation might occur, then disable and return back to the not-cancel-able mode. Checking a boolean state will practically impose no overhead. I will try to implement this and if I get into anything actually functional, will issue another pull request.


Reply to this email directly or view it on GitHub.

Member

normanmaurer commented Feb 8, 2014

Having write and flush seperated allows to easily implement pipelining. So a user call multiple times write and then flush to try to write everything to the socket with one syscall

Am 08.02.2014 um 04:01 schrieb Christos Fragoulides notifications@github.com:

@normanmaurer I get your point and I agree, most of the times there is no need to cancel a write so the potential overhead of checking would be a waste of resources.

But then, why should one write and then flush? I don't have a clear understanding of the reason for having to break the write operation in two steps, when the first step doesn't have a meaning without the second. The public API could simply expose a write() method that would be internally handled like writeAndFlush, and hide the flush() method. Is this backwards-compatibility related?

Another thought: could the ability to cancel writes be made optional, in order to reduce the checking overhead? This way, the user will have to enable cancellation support for the period of time during which a cancellation might occur, then disable and return back to the not-cancel-able mode. Checking a boolean state will practically impose no overhead. I will try to implement this and if I get into anything actually functional, will issue another pull request.


Reply to this email directly or view it on GitHub.

xfrag added a commit to xfrag/netty that referenced this pull request Feb 9, 2014

@normanmaurer normanmaurer referenced this pull request Feb 9, 2014

Closed

Cancel writes on 5.0 #2218

normanmaurer pushed a commit that referenced this pull request Feb 11, 2014

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Feb 11, 2014

Member

Will be handled by #2214

Member

normanmaurer commented Feb 11, 2014

Will be handled by #2214

@normanmaurer normanmaurer added this to the 4.0.16.Final milestone Feb 11, 2014

@xfrag xfrag deleted the xfrag:write_cancel branch Feb 12, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment