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
