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

On 24/09/10 08:30, Alex Rousskov wrote:
> 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.

In the general case the AsyncJobs that the Comm::Connection passes
through use it as a source of TCP link information. The ConnOpener and
ConnAcceptor are the main exceptions to this, since they have to create
the sockets and save information into the Comm::Connection.

>
>
>> 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.

Far from it. The above mentioned lookups and setups are all comm-level
stuff outside the scopes of HTTP parsing and forwarding (which is where
they are currently done).

IDENT is planned to become an AsyncJob spawned by ConnAcceptor to fill
the ident fields of Comm::Connection in parallel while the HTTP stuff
gets parsed. This will de-dupe the identical HTTP and HTTPS ident
lookups and permit its usage on ICP/HTCP queries.

EUI and TPROXY are currently sockopt() lookups. Pulling them out of the
request validation so the pre-forwarding access controls can use their
results would be a Good Thing. Being sockopts they are part of the Comm
actions and best done on socket arrival before the OS caches have a
chance to expire on busy systems.

TLS/SSL I'm thinking only of creating a Job which does the handshake.
This can be spawned and started from anywhere that needs SSL to begin on
a socket.

Such that a chain of jobs is created:
   caller -> Acceptor -> SslSetup -> callback 'done' handler.
   caller -> Opener -> SslSetup -> callback 'done' handler.

>
>> 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.
>

So I've noticed. I'm partway through de-duping and removing these bits
now. So far I have the http_port opener functions spawning a mix of
subscriptions for ConnAcceptor and passing them to your worker IPC
listeners which spawn the ConnAcceptors and pass them on.

Amos
Received on Thu Sep 30 2010 - 14:18:50 MDT