On 19/07/11 22:04, Tsantilas Christos wrote:
On 07/19/2011 05:51 AM, Amos Jeffries wrote:
On Mon, 18 Jul 2011 20:03:22 +0300, Christos Tsantilas wrote:
On 07/18/2011 01:04 PM, Tsantilas Christos wrote:
On 07/16/2011 07:11 AM, Amos Jeffries wrote:
On 16/07/11 07:43, Tsantilas Christos wrote:
But now I am hitting the following assertion:
assertion failed: Connection.cc:29: "fd < 0"
The later problem looks that it has to do with file descriptors of
the
listening sockets.

"fd < 0" indicates something is failing to call conn->close() when
abandoning an open socket.

NP: close() is reentrant. So components can and should always close()
when they are sure the FD/socket must no longer be used.

I'm not very certain about SMP listening sockets, which process(es)
are
safe to close() on reconfigure/shutdown? the unsafe ones must do fd=-1
to abandon the FD information explicitly before the conn object
destructs.

What situations are you hitting "fd < 0" Christos?

I am hitting this assertion on kids immediately after start.
Looks that the connection looses all references on its self and
deleted.
The socked of the connection is a listening socket.

When a kid starting get from the parent the filedescriptors of
listening sockets, and creates a Comm::Connection objects for these
filedescriptors.

What needed here is to assign the created Comm::Connection objects to
the related http_port_list object (to increase the refcount and keep
the connection open)

A way to implement the above is to use the ListeningStartedDialer
class implemented in client_side.cc file.

I am attaching a patch which solves this problem and allow smp-squid
start and serve HTTP requests, but there are similar or related bugs
in icp and snmp. When I am defining icp_port and snmp_port in
squid.conf the smp-squid does not start.

In this patch the dialer has no "conn" object to assign.

There is a Ipc::StartListeningCb::conn object. The
ListeningStartedDialer is a kid class of StartListeningCb. The conn
object exists and it is valid.
The patch is working but of course is incomplete. I post it just to
point the problem.

Currently the ListeningStartedDialer dialer is the only object we have
which maps the connection (or the FD better) to a listening port.



NOTE:
clientHttpConnectionsOpen() does "s->listenConn = new
Comm::Connection()" before starting IPC.
When dialling post-IPC happens
ListeningStartedDialer::portCfg->listenConn is still a ref of that
object.

I'm thinking:
If OpenListenerParams adds a member to hold the listenConn created by
clientHttpConnectionsOpen() it can be set with the response.fd in
Ipc::SharedListenJoined().

Just take a look inside Ipc::SharedListenJoined(), in the case of SMP
just sent to the kids the local address, flags and socket type, and
return. These are informations which can be send through IPC,pipes etc.
Unfortunately the OpenListenerParams copied with memcpy in a message
send thought ICP. How can we send a refcounted object like the
listenConn through a pipe? This will cause bugs similar to the bug 3264.
We can send only the FD.

I believe that the only we can do is to add a new parameter of type
Comm::Connection to the Ipc::JoinSharedListen function to pass the
listenConn object and add it to the related PendingOpenRequest.


We may or may not then have to do the c=new Comm::Connection() in IPC.

OK, maybe, we can just set the FD and other informations to the existing
listenConn...



The best point to set anything coming back anyway seems to be in
Ipc::SharedListenJoined() where the SharedlistenResponse,

It is the best, because looks that it will solve the related problems in
snmp and icp too...
But, at this time we do not have the listenConn object inside
Ipc::SharedListenJoined() and we do not have enough information to
retrieve it.
I will try to implement what I am proposing above (passing listenConn to
JoinsSharedListen...), it looks simple.
>

SharedListenResponse needs to use getInt(this->fd) or similar instead of
putPod/getPod(*this) anyway. Or at the very least give is a sub-struct
that can be documented as raw socket bytes and get*(&this->data_).
memcpy re-init of a whole class with methods straight from the socket
does not seem like a good idea.

Please see the patch in the bug 3264:
http://bugs.squid-cache.org/show_bug.cgi?id=3264
The bug described here is different to the bugs we are discussing here
but they are related. I believe just applying the patch I post here or a
similar is enough.


see attached patch. I've no idea if this will even build, but its what I
am hoping will work. It assumes the SharedListenResponse::conn --> SharedListenResponse::fd change from bug 3264 has also been made.


Amos
--
Please be using
  Current Stable Squid 2.7.STABLE9 or 3.1.14
  Beta testers wanted for 3.2.0.9
=== modified file 'src/ipc/SharedListen.cc'
--- src/ipc/SharedListen.cc	2011-05-13 08:13:01 +0000
+++ src/ipc/SharedListen.cc	2011-07-19 10:46:58 +0000
@@ -127,10 +127,8 @@
 
 void Ipc::SharedListenJoined(const SharedListenResponse &response)
 {
-    Comm::ConnectionPointer c = response.conn;
-
     // Dont debugs c fully since only FD is filled right now.
-    debugs(54, 3, HERE << "got listening FD " << c->fd << " errNo=" <<
+    debugs(54, 3, HERE << "got listening FD " << response.fd << " errNo=" <<
            response.errNo << " mapId=" << response.mapId);
 
     Must(TheSharedListenRequestMap.find(response.mapId) != TheSharedListenRequestMap.end());
@@ -138,22 +136,24 @@
     Must(por.callback != NULL);
     TheSharedListenRequestMap.erase(response.mapId);
 
-    if (Comm::IsConnOpen(c)) {
+    StartListeningCb *cbd = dynamic_cast<StartListeningCb*>(por.callback->getDialer());
+    Must(cbd && cbd->conn != NULL);
+    cbd->conn->fd = response.fd;
+
+    if (Comm::IsConnOpen(cbd->conn)) {
         OpenListenerParams &p = por.params;
-        c->local = p.addr;
-        c->flags = p.flags;
+        // XXX: these should be the same now. no need to copy. But maybe a verify check?
+        cbd->conn->local = p.addr;
+        cbd->conn->flags = p.flags;
         // XXX: leave the comm AI stuff to comm_import_opened()?
         struct addrinfo *AI = NULL;
         p.addr.GetAddrInfo(AI);
         AI->ai_socktype = p.sock_type;
         AI->ai_protocol = p.proto;
-        comm_import_opened(c, FdNote(p.fdNote), AI);
+        comm_import_opened(cbd->conn, FdNote(p.fdNote), AI);
         p.addr.FreeAddrInfo(AI);
     }
 
-    StartListeningCb *cbd = dynamic_cast<StartListeningCb*>(por.callback->getDialer());
-    Must(cbd);
-    cbd->conn = c;
     cbd->errNo = response.errNo;
     cbd->handlerSubscription = por.params.handlerSubscription;
     ScheduleCallHere(por.callback);

Reply via email to