Re: /bzr/squid3/trunk/ r9532: Regression Fix: Bug 2586: Memory leaks on reconfigure
On 02/23/2009 05:28 PM, Amos Jeffries wrote: revno: 9532 committer: Christos Tsantilas chtsa...@users.sourceforge.net branch nick: TRUNK timestamp: Tue 2009-02-24 01:18:23 +0200 message: Regression Fix: Bug 2586: Memory leaks on reconfigure The patch is not correct. Some of the objects freed here can be in use during reconfigure. Sigh, another design flaw in the current config handling. Exactly how are they in use, and which ones? NP: reconfigure and shutdown both call the free-up after all client connections are supposed to be dead. And all non-memory management components are already shutdown. 'reconfigure' global is available for selective preservation during reconfigure. I do not know whether the objects in scope of the patch are incorrectly maintained during the reconfigure, but it is possible that the patch would be correct if object maintenance is fixed. However, the whole idea that you should shutdown everything, delete virtually everything, and then create and configure from scratch is obsolete, IMO. Making reconfigure nearly identical to restart is simpler, but it is just not good enough for busy proxies. At this point, I do not know whether we should invest cycles in fixing leaks under the from scratch model or focus on migrating to live reconfiguration. Perhaps the answer is the reconfigure global you mentioned. This is something that should be discussed. Thank you, Alex.
Re: /bzr/squid3/trunk/ r9532: Regression Fix: Bug 2586: Memory leaks on reconfigure
revno: 9532 committer: Christos Tsantilas chtsa...@users.sourceforge.net branch nick: TRUNK timestamp: Tue 2009-02-24 01:18:23 +0200 message: Regression Fix: Bug 2586: Memory leaks on reconfigure The patch is not correct. Some of the objects freed here can be in use during reconfigure. Sigh, another design flaw in the current config handling. The configure/reconfigure/shutdown is a nightmare for many reasons. Exactly how are they in use, and which ones? The Config.ssl_client.sslContext is used by the http side sockets (forward.cc FwdState::initiateSSL method.) Maybe it is still in use during reconfigure. The peer_free is not really called . But if someone wants to fix peer leaks must convert the struct peer to a refcounted class (or cbdata class) and create a destructor to free the strings and ssl objects. The part of the patch for leaks in CacheManager::registerAction is OK. The part for mime.cc, I am not sure. NP: reconfigure and shutdown both call the free-up after all client connections are supposed to be dead. And all non-memory management components are already shutdown. If I am not wrong during reconfigure only the sockets listening for new connections closed. The other sockets (these which are opened by the client and these which are opened by the squid to remote servers) remains active. I think is not difficult to fix these leaks, but needs more work . 'reconfigure' global is available for selective preservation during reconfigure. Amos Regards, Christos
Re: /bzr/squid3/trunk/ r9532: Regression Fix: Bug 2586: Memory leaks on reconfigure
Alex Rousskov wrote: On 02/23/2009 05:28 PM, Amos Jeffries wrote: Sigh, another design flaw in the current config handling. Exactly how are they in use, and which ones? NP: reconfigure and shutdown both call the free-up after all client connections are supposed to be dead. And all non-memory management components are already shutdown. 'reconfigure' global is available for selective preservation during reconfigure. I do not know whether the objects in scope of the patch are incorrectly maintained during the reconfigure, but it is possible that the patch would be correct if object maintenance is fixed. However, the whole idea that you should shutdown everything, delete virtually everything, and then create and configure from scratch is obsolete, IMO. Making reconfigure nearly identical to restart is simpler, but it is just not good enough for busy proxies. If the store can be used by more than one squid processes there is a very simple solution: Shutdown the squid processes with old configuration (waiting all old sessions ended) and start new squid processes with the new configuration. I am wandering how difficult is to implement shared stores in squid... At this point, I do not know whether we should invest cycles in fixing leaks under the from scratch model or focus on migrating to live reconfiguration. Perhaps the answer is the reconfigure global you mentioned. This is something that should be discussed. Thank you, Alex.
Re: Sbuf review at r9331
- check that the function names in debugs() statements match their actual name (there's been quite a lot of shuffling) Please use debugs(..., HERE blah) for that sort of thing at levels 1. I've also just added MYNAME for prettier but identical use on the top two levels. Now done. -- /kinkie
Re: Associating accesses with cache.log entries
Not sure I follow. ctx_enter is called relatively late, in httpProcessReplyHeader and destroy_MemObject; I'd think for this case we need to have a request id associated quite early, probably around parseHttpRequest. Also, AFAICT Ctx_Descrs and Ctx_Current_Level isn't really a reliable way to identify what the context of a particular debug statement is. What am I missing? The most straightforward way that I can see to do this is to add an identifier to clientHttpRequest and pass that to debug where available... On 28/11/2008, at 12:02 PM, Henrik Nordstrom wrote: fre 2008-11-28 klockan 10:34 +1100 skrev Mark Nottingham: Agreed. How would you pass it into debug()? It looks like _db_print already takes variable length args, By adding it to the context already used by _db_print.. i.e. by extending ctx_enter to take the sequence number in addition to url. Regards Henrik -- Mark Nottingham m...@yahoo-inc.com
Re: Associating accesses with cache.log entries
ons 2009-02-25 klockan 12:10 +1100 skrev Mark Nottingham: What am I missing? The most straightforward way that I can see to do this is to add an identifier to clientHttpRequest and pass that to debug where available... That is what ctx_enter is about... There is not a single location where ctx_enter needs to be called, there is many.. Remember that Squid is a big bunch of event driven state machines, doing a little bit of processing at a time interleaved with many other unrelated things. ctx_enter indicates which state transition is currently being processed, ctx_leave when that state transition has completed waiting for next event (even if still at the same state..) So you need ctx_enter in quite many places, providing a reasonable trace of the processing within the state machine so far, based on whatever identifier the current small step is about. Each time the processing returns to the comm loop you are back at ctx level 0 with no context. Sometimes the ctx level may be quite high, having many loosely related state transitions in the trace, sometimes even almost completely unrelated requests. Most of the time the state machine starts with something directly related to a specific request (read/write on http sockets) however, but there is also many other kinds of state transitions like DNS, timers etc. Regards Henrik
