From dev-return-20808-apmail-apr-dev-archive=apr.apache.org@apr.apache.org Mon Jul 21 09:45:15 2008 Return-Path: Delivered-To: apmail-apr-dev-archive@www.apache.org Received: (qmail 62519 invoked from network); 21 Jul 2008 09:45:14 -0000 Received: from hermes.apache.org (HELO mail.apache.org) (140.211.11.2) by minotaur.apache.org with SMTP; 21 Jul 2008 09:45:14 -0000 Received: (qmail 27624 invoked by uid 500); 21 Jul 2008 09:45:13 -0000 Delivered-To: apmail-apr-dev-archive@apr.apache.org Received: (qmail 27568 invoked by uid 500); 21 Jul 2008 09:45:13 -0000 Mailing-List: contact dev-help@apr.apache.org; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Id: Delivered-To: mailing list dev@apr.apache.org Received: (qmail 27557 invoked by uid 99); 21 Jul 2008 09:45:13 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 21 Jul 2008 02:45:13 -0700 X-ASF-Spam-Status: No, hits=1.2 required=10.0 tests=SPF_NEUTRAL X-Spam-Check-By: apache.org Received-SPF: neutral (athena.apache.org: local policy) Received: from [209.85.132.249] (HELO an-out-0708.google.com) (209.85.132.249) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 21 Jul 2008 09:44:19 +0000 Received: by an-out-0708.google.com with SMTP id c35so338264anc.44 for ; Mon, 21 Jul 2008 02:44:43 -0700 (PDT) Received: by 10.100.95.19 with SMTP id s19mr1647281anb.65.1216633483248; Mon, 21 Jul 2008 02:44:43 -0700 (PDT) Received: by 10.100.143.9 with HTTP; Mon, 21 Jul 2008 02:44:43 -0700 (PDT) Message-ID: Date: Mon, 21 Jul 2008 11:44:43 +0200 From: "Sander Striker" To: "Mladen Turk" Subject: Re: Changing the order of cleanup for some core objects Cc: "APR Developer List" In-Reply-To: <48843999.1010804@apache.org> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <48843999.1010804@apache.org> X-Virus-Checked: Checked by ClamAV on apache.org On Mon, Jul 21, 2008 at 9:24 AM, Mladen Turk wrote: > Hi, > > For you that have track the discussion Bojan, Sander and myself took > about apr_reslist and pre_cleanup_register this will be familiar. > For others I'll give a quick overview of the problem and > API usage diversity. As an example, I'll use the socket API. > > > If you are creating the socket server you'll need at least the > listening socket and then accept the connections creating more > sockets. Since the accepted sockets gets created and destroyed > during the server lifetime, user must ensure the memory doesn't > leek, by destroying at least the accepted socket memory used. > Since we use pools, this can be achieved only by creating a separate > pool for each accepted socked that will be destroyed once the > socket is not needed any more. All that is pretty straighforward > and used in many implementations. > > Now the problem arises with the order of cleanups. > > Socket created with S = apr_socket_create(P) registers its > cleanup for pool P. The call for apr_socket_close(S) merely > calls that cleanup causing the underlaying OS socket to get closed. > > ... > S = apr_socket_create(P) > ... do something > ... with socket > apr_socket_close(S) -> calls socket_cleanup(S) > ... > > However if the apr_pool_destroy(P) gets called before > apr_socket_close call (somebody rise the signal, etc..) > the apr_pool_destroy call will cause the socket_cleanup(S) > call and the apr_socket_close(S) will be no-op , and everything > will behave as expected. > > Now, lets assume that socket S accepts connections. To keep > the memory usage constant the accept loop must clear all the memory > used so far: > > ... > S = apr_socket_create(P) > while (alive) { > C = apr_pool_create(P) > A = apr_socket_accept(S, C) > ... do something > ... with accepted socket A > apr_socket_close(A) > apr_pool_destroy(C) // No memory leaks > } > apr_socket_close(S) > ... > > So here we create sub-pool (C) of the pool (P) > and after we've done we destroy that pool to keep > the memory usage constant. > > However if the signal arises (somebody hit CTRL+C) > the whole sort of problems comes in due to the fact > that upper algorithm doesn't work any more, and the > order of objects destruction occurs. Not seeing that case TBH. Either the app has a signal handler and proper teardown logic, or it's a signal you can't catch and the kernel reclaims all resources anyway. > For example if we call apr_socket_close(S) while > the code had A sockets alive, any operation on the > socket A would fail with some OS specific return value. > > ... A = apr_socket_accept(S, C) > apr_socket_close(S) -> rv = apr_socket_read(A) > ... if (rv == ERROR) > ... apr_socket_close(A) > ... apr_pool_destroy(C) > ... ... > > However if instead apr_socket_close the interruption > was done by apr_pool_destroy(P) (socket S will be > closed by the cleanup callback) we cannot use the upper > code any more because: > > ... A = apr_socket_accept(S, C) > apr_pool_destroy(S) -> rv = apr_socket_read(A) > ... if (rv == ERROR) > ... apr_socket_close(A) > ... apr_pool_destroy(C) => !CORE > > The reason why the legitimate code cores is because the > pool from which the socket A was allocated was destroyed > before the actual cleanup for socket S was called. > > The user in this case must figure out the reason of > the error code from the apr_socket_read and in the > case the call failed because of the apr_pool_destroy made > on the parent pool it *must not* call its pool destroy > because it was destroyed already so you have double-free > problem. The fact that the parent pool is being destroyed means that that code shouldn't be executing anymore in the first place. > This is usually resolved either by some global variable, > by additional cleanup for each accepted socket (complete > waste of resources) I'll bite, even though it doesn't matter for this discussion: additional cleanup is not in all cases a complete waste of resources. Remember, a lot of cleanups fit inside the initial 8k chunk a pool starts with. > or something even more innovative ;) > > Using pre_cleanup for some objects that presumably have > child pools can resolve some of those issues, but it > definitely needs a thorough evaluation. > > Comments? More a question: what is the actual proposal? Cheers, Sander