Re: [PATCH] raw-FD related AsyncCall classes

On 11/23/2011 05:37 PM, Amos Jeffries wrote:
> On Wed, 23 Nov 2011 10:56:24 -0700, Alex Rousskov wrote:
>> On 11/23/2011 05:11 AM, Amos Jeffries wrote:

>>> and use it to ease
>>> the transition from cbdata objects to Jobs. It seems the big hitch
>>> currently to transitioning is that to use generic Unary/Nullary
>>> templates the object must be a fully transitioned AsyncJob so we end up
>>> with custom-coded dialers for each cbdata class type.
>>
>> I do not see how UnaryCbdataDialer "eases the transition", but it will
>> improve old-style code so I welcome its addition.
>>
>> IMO, the "big hitch" is in satisfying AsyncJob requirements. Once that
>> is done, adjusting callbacks should be very straightforward.
>>
>
> I was assuming that since AsyncJob is cbdata child and does cbdata
> validation prior to dialing the job members, that mirroring that check
> would be sufficient for other cbdata.
> - validate is performed before dialing the call and cbdata done() is
> used on destructing the call (dialed or not).
>
>
> The other core requirement that the async call handler has no
> expectation of timing, only of sequence. Seems to be met by the cbdata
> handlers assumption that the calling code must exit immediately, or move
> on to independent code operations not trusting the passed cbdata object
> or parameters.
>
> Have I missed anything?

I think what you are describing are handler/call[back] requirements. I
agree that they are pretty much met before or after this patch (or, at
least, fixing broken handlers to meet them is not related to this
discussion).

I was talking about satisfying AsyncJob class API requirements such as
how you start AsyncJobs, how you do not communicate with them directly
after start, how you must implement the doneAll() and the swangSong()
methods, etc. Those are non-trivial changes with difficult-to-understand
consequences.

>> canDial() should probably return cbdataReferenceValid(data) for call
>> debugging to work correctly and possibly to reduce call setup overheads.
>> Similarly, dial() should probably not check cbdataReferenceValid(data)
>> again because canDial() does.
>>
>
> Hm. Okay. Was trying to avoid complex/slow canDial(), but agreed. done.

I do not think you are adding complexity or slowing anything down by
_moving_ the "can we dial?" check to where it belongs. We should not be
calling canDial() just for the fun of it (and if we do, that is a
different bug outside your changes scope).

> I was not entirely clear what happens to the scheduled call if
> !canDial() code path. Drop the queue refcount and destruct?

The same thing that happens in your initial patch when you do not dial
in dial(): the call object is dequeued but not dialed and is eventually
destroyed as unused due to refcounting.

The difference is that one may be able to see what actually happened in
the debug log. Eventually, we may also stop enqueuing calls that cannot
be dialed at scheduling time, but it is not clear whether that is an
optimization or overhead (needs studying).

>>> - int fd; // raw FD which the call was about. use conn instead for
>>> new code.
>>> + int fd; /// FD which the call was about. Set by the caller
>>> creator. Use conn instead for new code.
>>
>> s/caller creator/caller/? The creator of the callback does not set the
>> fd value.
>
> The current ones using this do. See _comm_close() in comm.cc.

The _comm_close() code does not create callbacks. It creates two async
calls (commStartSslClose and comm_close_complete) and then updates and
schedules callbacks created by others (via ...->finish() calls).

> I would like the commCallCloseHandlers() not care about duplicating that
> effort. Just to schedule them all and finish.

Not sure what you mean. I was only commenting on the unclear fd data
member description, nothing else.

>> Also, since not all new code should use Connection after your changes, I
>> would replace "Use conn instead for new code" with "Use conn if
>> available" or something like that. I have to say that your changes
>
> ?? missing text?

Sorry. Please ignore the "I have to say that your changes" part. IIRC, I
was going to say that it becomes increasingly difficult to know when one
should use fd and when one should use conn, but I already complained
about it, and I do not want to argue about that again.

Thank you,

Alex.
Received on Thu Nov 24 2011 - 01:35:49 MST