On Thu, Jan 21, 2010 at 06:08:41PM -0600, Shawn Walker wrote: > On 01/21/10 06:05 PM, [email protected] wrote: > >On Thu, Jan 21, 2010 at 05:55:11PM -0600, Shawn Walker wrote: > >>On 01/21/10 05:51 PM, [email protected] wrote: > >>>The enable logic is pretty straight forward, and all cases are the same. > >>>We could turn this into a function, but then we're adding another stack > >>>frame for a pretty simple task. > >> > >>My only concern with the simple bit is that it's easy to get wrong. > >> > >>I recently started trying to write a fairly large function in the > >>client.api and realised that trying to support cancellation was a > >>nightmare, especially when relying on other api functions. > > > >Well, this wad aims to fix some of that. I've put an assertions in > >methods that expect the lock to be held by their callers. I switched > >the cancel lock to an NRLock, and I've made it explict now that all > >callers that wish to make an operation cancelable must hold the activity > >lock for the duration of the operation that's to be canceled. If > >there's something else you'd like me to clarify, I'm happy to try. The > >only portion of the lock dance that could be abstracted into a function > >is: > > > > acquire cancel_lock > > set can_be_canceled<- True > > release cancel_lock > > > >IMO, this process is straight forward enough that it doesn't need to be > >in a function, but it's certainly not hard to make it one. > > I have no real objections, it was more of a general comment about > the difficulty of implementing cancellation support in a function. > Hopefully this change will make that easier.
I totally agree that this isn't straight forward. I think the difficulty is compounded by the fact that we had no explicitly documented definition of what our existing locks were supposed to protect, and the fact that we've made piecemeal additions to the API. I hope that my changes make things a bit more explicit and consistent. If you think that adding an __enable_cancel() will help, I can add the function. My primary concerns for not doing so are twofold: a. It adds an extra stack frame, which makes SPARC go slower b. You can explicitly see the lock order in functions this way. (First activity lock, then cancel lock). -j _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
