Re: [PATCH] Comm cleanup (Async ConnOpener)

On 07/20/2010 07:53 AM, Amos Jeffries wrote:

> Ew. A call to schedule a call?

The "ew" part applies to the sync callback. Since we cannot fix that
(because it is outside your project scope), there is nothing wrong with
making that callback async.

> Sorry, I responded to that without taking another look at the code.
>
> ConnOpener::swanSong() actually calls the shared and public
> Connection::close() which calls comm_close() and unsets the FD.

In the latest patch I have seen, ConnOpener::swanSong calls close(2) and
then manipulates Comm state directly:

+ // it never reached fully open, so abort the FD
+ close(solo->fd);
+ fd_table[solo->fd].flags.open = 0;

I do not know what you mean by "shared Connection::close()" but that is
not what is happening above.

> Do you want to audit the code at this stage or wait till I finish that
> next level of rollout into the Servers' ?

IMHO, one should post the current patch whenever discussing changes made
to address the previous review points. Otherwise, it is difficult to
verify that the comments were addressed [correctly]. You can always
mark the patch as "not final" and reviewers can always skip it.

>>>> * FwdState::unregister() used to prevent FwdState from closing the
>>>> unregistered connection. The new code does not. Was the old code buggy?
>>>>
>>> Only in Comm::Connection semantics. Added an alternative version and
>>> deprecated the old unregister(int) with a hack to keep it going on the
>>> old semantics.
>>
>>
>> I will try to remember to revisit this when you post the patch with the
>> changes described above, but I may forget.
>>
>
> New code is this pair of methods:
>
> void
> FwdState::unregister(Comm::ConnectionPointer conn)

Please use const references for refcounted pointer parameters.

> {
> debugs(17, 3, HERE << entry->url() );
> assert(serverConnection() == conn);
> assert(conn->isOpen());
> comm_remove_close_handler(conn->fd, fwdServerClosedWrapper, this);
> }

> // Legacy method to be removed in favor of the above as soon as possible
> void
> FwdState::unregister(int fd)
> {
> debugs(17, 3, HERE << entry->url() );
> assert(fd == serverConnection()->fd);
> assert(fd > -1);
> comm_remove_close_handler(fd, fwdServerClosedWrapper, this);
> serverConnection()->fd = -1;
> }

Why not just call the new method from the legacy one instead of having
partially duplicated code? If the code semantics is supposed to be
different, let's not name the methods the same.

> Yes, I do see that the fd = -1 was critical to the old code semantics.
>
> This is a problem. unregister() can prevent the fwdServerClosedWrapper
> being called from that point on. From a days running of Squid that seems
> to be sufficient.

The new unregister() code neither prevents fwdServerClosedWrapper from
being called (because the call may already be scheduled when you call
comm_remove_close_handler) nor does it prevent FwdState from closing the
connection later (because conn->fd is still not negative). Thus, I do
not see how the introduction of the new unregister() method addresses my
concerns, but it is possible that it will become clear when the whole
patch is available.

Should the new unregister() set FwdState server connection pointer
(paths[0]) to NULL?

> The better solution may be to not call ->close() or comm_close()
> directly anymore from FwdState or any of the Servers. Relying on the
> Comm::Connection to do its own closing on destruct.

I doubt. If we want to close, we should close. If the connection pointer
is shared by several objects, it would be difficult to _force_
connection destruction (and cleanup closure) at any given point.

The fact that the Connection object state is often inconsistent with the
Comm state is a bug that will probably bite us a few times. We should
consider rewriting Connection::isOpen to ask Comm whether it is really
open. That will not solve the "stale fd" problem but at least will get
us back on par with the old known-to-work code. Currently, the patch
essentially creates two isOpen methods that may yield different results:
one from Comm and one from Connection point of view.

HTH,

Alex.
Received on Tue Jul 20 2010 - 16:06:30 MDT