Re: [PATCH] comm cleanup mk8 - src/comm only

On 09/23/2010 11:10 AM, Amos Jeffries wrote:
> On 24/09/10 04:39, Alex Rousskov wrote:
>> On 09/23/2010 09:46 AM, Amos Jeffries wrote:
>>> On 23/09/10 04:49, Alex Rousskov wrote:
>>>> On 09/22/2010 08:28 AM, Amos Jeffries wrote:
>>>>> On 21/09/10 10:00, Alex Rousskov wrote:
>>>>>> On 09/19/2010 05:35 AM, Amos Jeffries wrote:
>>>>
>>>
>>> Patch including the changes requested to date attached.
>>>
>>>
>>
>>>> AFAIK, comm_close() will remove those handlers automagically because it
>>>> will call them with COMM_ERR_CLOSING. However, our code needs to make
>>>> sure that it can handle such callbacks until swanSong() is called.
>>>
>>> K. Thats what I'm slightly worried about. This is inside swanSong. So by
>>> the time this set of comm_close scheduled ones occur the swanSong is
>>> over and done with. Is the cancel() enough to make dialing drop and
>>> skip?
>>> connect() which they all lead back to starts with a Must() which will
>>> trigger after swanSong() even if the job object itself is still alive.
>>>
>>> It won't loop more than once, but scheduling is very a sub-optimal way
>>> to kill them.
>>
>> Nothing can happen to the job after swanSong() because the job is
>> deleted after it signs its last song. A dead job cannot receive any
>> callbacks. You may but do not have to cancel Comm callbacks because they
>> will not reach the job anyway. Calling comm_close should be enough to
>> keep Comm state consistent.
>>
>> Specifically:
>>
>>> + // rollback what we can from the job state
>>> + if (conn_ != NULL && conn_->isOpen()) {
>>
>> .
>>
>>> + // drop any handlers now to save a lot of cycles later
>>> + commSetSelect(conn_->fd, COMM_SELECT_WRITE, NULL, NULL, 0);
>>
>> _comm_close does that if there is a write callback.
>>
>>> + commSetTimeout(conn_->fd, -1, NULL, NULL);
>>
>> _comm_close does that.
>>
>>> + // it never reached fully open, so abort the FD
>>> + conn_->close();
>>
>> That must be enough.
>>
>>
>>
>> The only remaining thing is moving Comm::ConnOpener::connected() to
>> Connection::connected() but that is minor. Could be useful for those
>> cases were we still connect without using your new code though.
>>
>
> Ah thats what you meant. Connection is trying to stay as close to a dumb
> storage type as possible.
> The lookups fde state changes needed by connected() would make it
> heavier. I'd avoid it's close() as well if I could.

In general, the class should manage its members. Dumb storage leads to
leaks, races, management code duplication, and other nasties. For
example, you should be happy that your Connection does not let the
descriptor leak, even if it creates transition headaches.

I suspect we need smarter Connection, not dumber one. We also need to
move from dumb int and fde to a Descriptor class(es) of sorts. All of
that is outside your patch scope though.

> NP: long-term the plan is to have TPROXY, TLS/SSL, EUI and IDENT to the
> remote end of the new connection also started at the connected() point.
> Based on Connection::flags.

A giant switch() creating high-level objects inside some low-level code
based on flags does not excite me. I would rather creating
protocol-specific transactions and let them manage the connecting step,
but this may be too abstract and remote to be worth arguing about.

> With the equivalent client-facing ones in ConnAcceptor.

Client-side Acceptor is rather different. It needs to create
protocol-specific "connection managers" which, in turn, create
protocol-specific transaction classes for each incoming request on the
connection. Client-side code tries to do a lot of that already, but does
not get the details right.

Cheers,

Alex.
Received on Thu Sep 23 2010 - 20:31:01 MDT