Re: [PATCH] AsyncCall and CommCalls changes for cleanup-comm work.

On 10/05/2010 03:33 AM, Amos Jeffries wrote:
> On 05/10/10 16:50, Alex Rousskov wrote:
>> On 09/23/2010 10:05 AM, Amos Jeffries wrote:

>>> class CommCommonCbParams
...
>>> public:
>>> void *data; // cbdata-protected
>>> - int fd;
>>> + Comm::ConnectionPointer conn;
>>> + int fd; // raw FD from legacy calls. use conn instead.
>>
>> Having both fd and conn is bad, but having them pretty much undocumented
>> makes it impossible for me to review the patch: I cannot review the
>> sometimes complex interactions between the two in the rest of the patch
>> because I do not understand when we should have none, one, the other,
>> and both members. Did I miss some documentation piece explaining this?

> All the new code (ConnAcceptor, ConnOpener, IPC, write calls, read
> callbacks) uses conn. The code I have not got to transitioning yet (read
> callers/timeout/close calls) sets and reads 'fd'.
>
> The 'complex interactions' only exists on dialing the IOCB. Where it
> makes sure that if one is set by the scheduling code the other is a mirror.
>
> The special case (for now) is IOACB dialer where fd contains the (mostly
> unused listening socket FD) and conn contains the newely accepted
> connection descriptor.

I am sorry, but I believe this is just too messy. We are going two steps
backwards with a promise of one step forward some time later. I hope we
can find a better alternative.

There are probably several acceptable options here. Assuming CommCalls
should use Connection now, the options I can think of are:

A) Remove the fd data member from CommCommonCbParams. Add/leave the conn
member. Add fd() wrappers for the old, fd-using code. Here is a _sketch_
for the wrappers:

   void fd(int anFd) { Must(!conn); conn = new Connection(anFd); }
   int fd() const { return !conn ? -1 : conn->fd; }

Please note that this option alone does not solve the conn-closing-fd
problem described at the end of this email.

B) Remove fd data member from CommCommonCbParams. Add conn member.
Update all users to use conn instead of fd.

In either case, we must describe the new "conn" member because it is
obviously being interpreted very differently by patch readers. The right
definition would be a good starting point.

>>> @@ -65,12 +69,6 @@
>>> {
>>> public:
>>> CommAcceptCbParams(void *aData);
>>> -
>>> - void print(std::ostream &os) const;
>>> -
>>> -public:
>>> - ConnectionDetail details;
>>> - int nfd; // TODO: rename to fdNew or somesuch
>>> };
>>
>> Why was nfd removed?
>
> Because having two FD and a conn was worse than one fd and a conn.

CommAcceptCbParams class has one "new fd" and no conn members so the
above argument does not apply. The number of "old fd" and conn members
in the parent class is irrelevant due to completely different fd member
meanings/purposes and just general encapsulation reasons.

> Also most of the local-IP/remote-IP/flags fetching details are now
> hidden inside ConnAcceptor::connected() instead of duplicated in the
> callback handlers. When the shuffling is complete 'fd' field will die
> too and the callers will use their stored listenConn (instead of fd) and
> the parameter io.conn where necessary instead of fd and nfd respectively.

The _accept_ callback must return a new connection (or descriptor). No
other Comm call returns that. Thus, CommAcceptCbParams and not its
parent should have "newFd", "newConn", or and equivalent member,
regardless of where the listening descriptor (on which the new one is
accepted) is stored.

If I understand your changes and explanations correctly, you are
removing a needed member. We should not reuse the "fd" or "conn" member
in the parent here because the parent member has a different meaning
(and also because some other code/comments declare that member
transitional/temporary).

> > Is it not specific to the accept callback?
>
> It is/was. Fully superseded by the conn member. Which is currently still
> called "details" parameter to IOACB.

Conn in parent is for the listening connection, not for the newly
accepted one. If conn in parent is not needed because future users will
store and find the right conn using some other means, then the parent
conn can be removed, but this should not affect the "nfd" or "newConn"
in this class because it will continue to be needed.

>>> // connect parameters
>>> @@ -80,11 +78,6 @@
>>> CommConnectCbParams(void *aData);
>>>
>>> bool syncWithComm(); // see CommCommonCbParams::syncWithComm
>>> -
>>> - void print(std::ostream &os) const;
>>> -
>>> -public:
>>> - DnsLookupDetails dns;
>>> };
>>
>> Where did the dns details go? Are they now supplied using some other API?
>
> They jumped out of Comm layer, up past ServerStateData and FwdState then
> sideways into peer_select.cc. Connection opening is post route selection
> and thus far too late for DNS resolving.

Are you saying that Comm connect no longer does DNS lookups? If yes,
then it is OK to remove DnsLookupDetails from the callback, of course,
but we need to make sure that the old code using those details is
properly transitioned. IIRC, that old code is not specific to
ServerStateData and FwdState you have mentioned, and not all connections
need to select a peer.

> + // transition only: read/write legacy code does not know about conn, it just sets FD
> + if (fd >= 0) {
> + if (conn == NULL)
> + conn = new Comm::Connection;
> + if (conn->fd != fd)
> + conn->fd = fd;
> + }

In general, if the recipient of this callback does not store this newly
created conn, then the new conn will close the connection at the end of
the call back, right? That does not sound like the desired outcome!

Specifically, according to your comment above, the "read/write legacy
code does not know about conn", and so it will see its connections
closed immediately after the I/O callback ends?

Thank you,

Alex.
Received on Wed Oct 06 2010 - 04:15:27 MDT