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

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.

Thank you,

Alex.
Received on Thu Sep 23 2010 - 16:39:41 MDT