Re: [RFC PATCH] Re: PAC serving

On 07/14/2010 08:02 AM, Amos Jeffries wrote:
> Since no description beats the real code here is the current patch I have.
>
> If you guys can't find any other major issues with this I'll go ahead
> and make it build and test it out.

I have some comments, but they should not stop you from running and
testing the code, of course:

* Never call swanSong() directly. If you are outside an AsyncCall
handler, and want to kill the job, then call deleteThis(). If you are
inside an AsyncCall handler, you have several options for job termination:

  - Call mustStop(reason) for errors that require further processing in
the same method(s) chain, below/after the mustStop() call. Efficient.

  - Throw (via Must or directly) for errors that do not require further
processing in the same method(s) chain, below/after the mustStop() call.
Inefficient but simple and allows exiting from deeply nested method calls.

  - Otherwise, just finish the call. Your doneAll() should return true
and the job will terminate successfully.

swanSong() will be called automatically in all of these cases when the
job is being terminated. It is a general cleanup method, like a
destructor. The only difference is that a destructor must not throw.

* Do not assume swanSong() is called in some perfectly nice job state.
The job code or the code it calls may throw at any time after start()
was called. The entry may be gone, the Abort may have been called, the
fd may have been closed, etc.

* Never call deleteThis() in contexts other than those documented above.
It is a hack for the old-style code. You can avoid it and other
old-style special precautions altogether if you convert sync calls into
async ones. This is especially easy for old-style calls that have only
one parameter ("data") or two simple parameters.

* In swanSong, always call swanSong() of the parent, after you are done
cleaning up your job. It does not matter whether the [current] parent
swanSong() does nothing.

* If a job does Comm I/O, it probably needs a Comm closing handler.

* To start a job, use AsyncStart. Do not call start() directly. There
are examples in the code.

* Starting a job from a Comm handler is fine, but that handler should
not be a part of the job you are starting. Start the job in the caller
and let the job register its needs with Comm...

* If a job does not have a doneAll() method implemented, it is probably
buggy. Any job must know what it wants to accomplish. Please note that
doneAll() is for defining the successful termination goal/condition.
Errors are handled by mustStop() or throw, as discussed above.

Similarly, if your doneAll() implementation looks like "return isDone;",
you are doing it wrong. Compute the condition directly rather than
expecting various job methods to maintain some isDone variable correctly.

* Ask yourself what the user will see/experience when the job throws,
which could happen as early as in the start() method (technically, it
can happen even earlier, in the job creation or creator code). Are you
OK with that?

HTH,

Alex.
Received on Wed Jul 14 2010 - 21:35:22 MDT