On 11 July 2014 at 1:13:30 am, Radim Kubacki (radim.kuba...@gradleware.com) wrote:
Moving the discussion to dev list as suggested by Daz. To Daz: you've got it right. That is my plan (with some small deviations possible if I find better ideas). On Thu, Jul 10, 2014 at 2:22 AM, Luke Daley <luke.da...@gradleware.com> wrote: On 10 July 2014 at 2:30:58 am, Radim Kubacki (radim.kuba...@gradleware.com) wrote: Hi, here are some comments to https://github.com/gradle/gradle/blob/master/design-docs/tooling-api-cancellation.md I think Adam will have comments when he gets back. Since he is not there I will share it this way. Story: Daemon waits short period of time for cancelled operation to complete I think we need to distinguish between 'request to stop the daemon' and 'cancel the build' which is not the case in current spec. To me it’s implied in this story that we are changing the behaviour of —stop to (at least at this stage) act as a cancellation request. It’s odd that Adam didn’t explicitly state that `—stop` needs to be changed to not call requestForcefulStop() or that —stop should be the same as cancelling (at this stage). If the intent really is to have —stop act like cancellation then I don’t think we need the distinction you are pointing out here. The implementation plan as it’s currently written doesn’t make sense to me. I don’t understand the reasoning behind changing the semantics of “stopping” to only shutdown the daemon if it doesn’t finish in time. Stopping should mean stopping. I believe we should treat them as different use-cases: stop should always stop the daemon. For cancel it is desirable to improve the implementation to the point where it can stop current build and keep the daemon running (this will give us the advantage of long-living warmed up daemon). I think this must have been an oversight by the spec writer. The behaviour of —stop should not change. May as well assume this until we hear differently. - 'request to stop the daemon' is now implemented in DaemonStateControl.requestForcefulStop(). This is called from client side using DaemonClient.stop() where Stop command is sent to server that processes it in its StopHandler. It can break an existing connection for the command that can be running at the moment and will cause the daemon server to exit. This works well for CLI call 'gradle --stop' and it is good enough as the basic implementation of cancellation - 'cancel the build': we don't want to stop the server if it is possible to stop the execution in some soft way (and within reasonable time). If this fails we can either ignore the cancel event or stop the daemon as described above. The story explicitly calls for shutting down the daemon if it doesn’t finish in time. Isn’t that the whole point of this story? It does and I will do it this way. Just wanted to point out that there are other alternatives too. I guess I will add DaemonStateControl.cancelBuild() and call it when Cancel command is sent from daemon client to server - only one command can be run by DaemonStateControl so I need this new asynchronous entry. Would it really be non blocking? Wouldn’t it be easier to have cancelBuild() block until things are stopped/cancelled? I'm not sure I understand you. Things may not stop/cancell (internal error or some other reason). You said that cancelBuild() needs to be non blocking. I’m trying to point out here that I don’t see why. Why can’t cancelBuild() only return once the build is indeed cancelled (at this point, either build stopped in time or we issued a stop request). And I will add some cancellation context to DaemonCommandExecution as a way of communication between initiated build and cancel processing. Cancel command needs to make it known that previously started Build command should be cancelled. The cancel state should be reset at the end of command execution. Then the daemon can be used to run another build. If the build is not cancelled/finished in a timely manner the fallback to DaemonStateControl.requestForcefulStop() will be used and the daemon will stop. This seems like more than you need. Why do you need to communicate with the initiated build for this story? Not this story but the one immediately following will: Task graph execution is aborted when operation is cancelled. I'm trying to think about that too. I’d suggest dealing this when you get to that story. I always have the most success when I do the practical minimum required for a particular story. If you can block in cancelBuild(), I’d add this to DaemonStateControl… // returns false if the daemon is already idle and the callback was not registered. boolean onIdle(Runnable callback); cancelBuild() would use this to manage only calling requestForcefulStop() if the build doesn’t stop in time. Sounds interesting. OK, I'll do some prototyping. — Luke Daley http://www.gradleware.com On Thu, Jul 10, 2014 at 2:22 AM, Luke Daley <luke.da...@gradleware.com> wrote: On 10 July 2014 at 2:30:58 am, Radim Kubacki (radim.kuba...@gradleware.com) wrote: Hi, here are some comments to https://github.com/gradle/gradle/blob/master/design-docs/tooling-api-cancellation.md I think Adam will have comments when he gets back. Since he is not there I will share it this way. Story: Daemon waits short period of time for cancelled operation to complete I think we need to distinguish between 'request to stop the daemon' and 'cancel the build' which is not the case in current spec. To me it’s implied in this story that we are changing the behaviour of —stop to (at least at this stage) act as a cancellation request. It’s odd that Adam didn’t explicitly state that `—stop` needs to be changed to not call requestForcefulStop() or that —stop should be the same as cancelling (at this stage). If the intent really is to have —stop act like cancellation then I don’t think we need the distinction you are pointing out here. The implementation plan as it’s currently written doesn’t make sense to me. I don’t understand the reasoning behind changing the semantics of “stopping” to only shutdown the daemon if it doesn’t finish in time. Stopping should mean stopping. - 'request to stop the daemon' is now implemented in DaemonStateControl.requestForcefulStop(). This is called from client side using DaemonClient.stop() where Stop command is sent to server that processes it in its StopHandler. It can break an existing connection for the command that can be running at the moment and will cause the daemon server to exit. This works well for CLI call 'gradle --stop' and it is good enough as the basic implementation of cancellation - 'cancel the build': we don't want to stop the server if it is possible to stop the execution in some soft way (and within reasonable time). If this fails we can either ignore the cancel event or stop the daemon as described above. The story explicitly calls for shutting down the daemon if it doesn’t finish in time. Isn’t that the whole point of this story? I guess I will add DaemonStateControl.cancelBuild() and call it when Cancel command is sent from daemon client to server - only one command can be run by DaemonStateControl so I need this new asynchronous entry. Would it really be non blocking? Wouldn’t it be easier to have cancelBuild() block until things are stopped/cancelled? And I will add some cancellation context to DaemonCommandExecution as a way of communication between initiated build and cancel processing. Cancel command needs to make it known that previously started Build command should be cancelled. The cancel state should be reset at the end of command execution. Then the daemon can be used to run another build. If the build is not cancelled/finished in a timely manner the fallback to DaemonStateControl.requestForcefulStop() will be used and the daemon will stop. This seems like more than you need. Why do you need to communicate with the initiated build for this story? If you can block in cancelBuild(), I’d add this to DaemonStateControl… // returns false if the daemon is already idle and the callback was not registered. boolean onIdle(Runnable callback); cancelBuild() would use this to manage only calling requestForcefulStop() if the build doesn’t stop in time. — Luke Daley http://www.gradleware.com
