couchdb-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Damien Katz <dam...@apache.org>
Subject Re: svn commit: r763858 - in /couchdb/trunk: share/www/script/test/ src/couchdb/
Date Fri, 10 Apr 2009 15:15:02 GMT
If you don't trap exits, then the default OTP behavior is to propagate  
linked exit messages to other child linked process, killing the entire  
process tree. The exception is for linked normal exits, which have no  
effect on non-trapping gen_server processes. They are just ignored. I  
think it's to allow optional manual propagation in the terminate  
handler, but I want it to be automatic.

Previously, when the couch_file processes receive a exit 'normal', it  
ignores it exit. Which is why I had it trapping exits before, so that  
it acted on the normal exit messages and shut down quietly. But if it  
got a 'kill' message, it would do the infamous process error dump  
instead of shutdown quietly.

Yesterday I introduced a regression earlier because I thought the  
files were getting cleaned up after I turned off trap exits in  
couch_file, but the tests for how many os_files are open was removed,  
so I didn't notice.

Now what I've done is make it so the gen_server processes exiting  
normally will now propagate a shutdown exit to any linked processes.  
This gives the desired behavior of automatic cleanup regardless how  
the process tree terminates..

The couch_ref_counter is still required. It makes sure the file stays  
open for any DB readers even when there is a compaction. Otherwise,  
any time there was a file compaction, any readers or writers in middle  
of a transaction would get a error as they tried to use a closed file  
descriptor.

I added back in the os_files test and I think know that I understand  
how the OTP stuff is supposed to work (you must explicitly cleanup  
linked process in the normal case) it's now solid.

-Damien


On Apr 10, 2009, at 10:40 AM, Adam Kocoloski wrote:

> Hi Damien, I see that the basic idea here is to terminate processes  
> linked to a couch_db, couch_db_updater, couch_ref_counter,  
> couch_server, or couch_view process even if the processes is doing a  
> normal termination.  I have a couple of questions:
>
> * where do the 'EXIT' messages handled by couch_file come from?  The  
> process doesn't trap exits anymore.
>
> * Is the couch_ref_counter dance still required?
>
> * should we look into one_for_all supervision as an alternative way  
> to achieve this effect?
>
> Best, Adam
>
> On Apr 9, 2009, at 10:21 PM, damien@apache.org wrote:
>
>> Author: damien
>> Date: Fri Apr 10 02:21:37 2009
>> New Revision: 763858
>>
>> URL: http://svn.apache.org/viewvc?rev=763858&view=rev
>> Log:
>> Fixes for leaked file handles, with test.
>>
>> Modified:
>>   couchdb/trunk/share/www/script/test/basics.js
>>   couchdb/trunk/share/www/script/test/stats.js
>>   couchdb/trunk/src/couchdb/couch_db.erl
>>   couchdb/trunk/src/couchdb/couch_db_updater.erl
>>   couchdb/trunk/src/couchdb/couch_file.erl
>>   couchdb/trunk/src/couchdb/couch_ref_counter.erl
>>   couchdb/trunk/src/couchdb/couch_server.erl
>>   couchdb/trunk/src/couchdb/couch_util.erl
>>   couchdb/trunk/src/couchdb/couch_view.erl
>>   couchdb/trunk/src/couchdb/couch_view_group.erl
>>
>> Modified: couchdb/trunk/share/www/script/test/basics.js
>> URL: http://svn.apache.org/viewvc/couchdb/trunk/share/www/script/test/basics.js?rev=763858&r1=763857&r2=763858&view=diff
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> =====================================================================
>> --- couchdb/trunk/share/www/script/test/basics.js (original)
>> +++ couchdb/trunk/share/www/script/test/basics.js Fri Apr 10  
>> 02:21:37 2009
>> @@ -29,10 +29,10 @@
>>    if (debug) debugger;
>>
>>    // creating a new DB should return Location header
>> -    xhr = CouchDB.request("DELETE", "/new-db");
>> -    xhr = CouchDB.request("PUT", "/new-db");
>> -    TEquals("/new-db",
>> -      xhr.getResponseHeader("Location").substr(-7),
>> +    xhr = CouchDB.request("DELETE", "/test_suite_db");
>> +    xhr = CouchDB.request("PUT", "/test_suite_db");
>> +    TEquals("/test_suite_db",
>> +      xhr.getResponseHeader("Location").substr(-14),
>>      "should return Location header to newly created document");
>>
>>    TEquals("http://",
>>
>> Modified: couchdb/trunk/share/www/script/test/stats.js
>> URL: http://svn.apache.org/viewvc/couchdb/trunk/share/www/script/test/stats.js?rev=763858&r1=763857&r2=763858&view=diff
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> =====================================================================
>> --- couchdb/trunk/share/www/script/test/stats.js (original)
>> +++ couchdb/trunk/share/www/script/test/stats.js Fri Apr 10  
>> 02:21:37 2009
>> @@ -56,7 +56,8 @@
>>          value: max.toString()}],
>>
>>        function () {
>> -          var files_open = requestStatsTest("couchdb",  
>> "open_databases").current;
>> +          var dbs_open = requestStatsTest("couchdb",  
>> "open_databases").current;
>> +          var files_open = requestStatsTest("couchdb",  
>> "open_os_files").current;
>>          for(var i=0; i<max+1; i++) {
>>            var db = new CouchDB("test_suite_db" + i);
>>            db.deleteDb();
>> @@ -70,7 +71,8 @@
>>            var db = new CouchDB("test_suite_db" + i);
>>            db.deleteDb();
>>          }
>> -          T(files_open == requestStatsTest("couchdb",  
>> "open_databases").current);
>> +          T(dbs_open == requestStatsTest("couchdb",  
>> "open_databases").current);
>> +          T(files_open == requestStatsTest("couchdb",  
>> "open_os_files").current);
>>        })
>>    },
>> };
>>
>> Modified: couchdb/trunk/src/couchdb/couch_db.erl
>> URL: http://svn.apache.org/viewvc/couchdb/trunk/src/couchdb/couch_db.erl?rev=763858&r1=763857&r2=763858&view=diff
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> =====================================================================
>> --- couchdb/trunk/src/couchdb/couch_db.erl (original)
>> +++ couchdb/trunk/src/couchdb/couch_db.erl Fri Apr 10 02:21:37 2009
>> @@ -677,7 +677,8 @@
>>    couch_ref_counter:add(RefCntr),
>>    {ok, Db}.
>>
>> -terminate(_Reason, _Db) ->
>> +terminate(Reason, _Db) ->
>> +    couch_util:terminate_linked(Reason),
>>    ok.
>>
>> handle_call({open_ref_count, OpenerPid}, _,  
>> #db{fd_ref_counter=RefCntr}=Db) ->
>>
>> Modified: couchdb/trunk/src/couchdb/couch_db_updater.erl
>> URL: http://svn.apache.org/viewvc/couchdb/trunk/src/couchdb/couch_db_updater.erl?rev=763858&r1=763857&r2=763858&view=diff
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> =====================================================================
>> --- couchdb/trunk/src/couchdb/couch_db_updater.erl (original)
>> +++ couchdb/trunk/src/couchdb/couch_db_updater.erl Fri Apr 10  
>> 02:21:37 2009
>> @@ -37,7 +37,9 @@
>>    Db2 = refresh_validate_doc_funs(Db),
>>    {ok, Db2#db{main_pid=MainPid}}.
>>
>> -terminate(_Reason, _Db) ->
>> +
>> +terminate(Reason, _Srv) ->
>> +    couch_util:terminate_linked(Reason),
>>    ok.
>>
>> handle_call(get_db, _From, Db) ->
>>
>> Modified: couchdb/trunk/src/couchdb/couch_file.erl
>> URL: http://svn.apache.org/viewvc/couchdb/trunk/src/couchdb/couch_file.erl?rev=763858&r1=763857&r2=763858&view=diff
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> =====================================================================
>> --- couchdb/trunk/src/couchdb/couch_file.erl (original)
>> +++ couchdb/trunk/src/couchdb/couch_file.erl Fri Apr 10 02:21:37 2009
>> @@ -311,7 +311,7 @@
>>    end.
>>
>>
>> -terminate(_Reason, _Fd) ->
>> +terminate(_Reason, _Fd) ->
>>    ok.
>>
>> track_stats() ->
>> @@ -359,5 +359,5 @@
>> code_change(_OldVsn, State, _Extra) ->
>>    {ok, State}.
>>
>> -handle_info(foo, _Fd) ->
>> -    ok.
>> +handle_info({'EXIT', _, Reason}, Fd) ->
>> +    {stop, Reason, Fd}.
>>
>> Modified: couchdb/trunk/src/couchdb/couch_ref_counter.erl
>> URL: http://svn.apache.org/viewvc/couchdb/trunk/src/couchdb/couch_ref_counter.erl?rev=763858&r1=763857&r2=763858&view=diff
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> =====================================================================
>> --- couchdb/trunk/src/couchdb/couch_ref_counter.erl (original)
>> +++ couchdb/trunk/src/couchdb/couch_ref_counter.erl Fri Apr 10  
>> 02:21:37 2009
>> @@ -49,7 +49,8 @@
>>    {ok, #srv{referrers=Referrers}}.
>>
>>
>> -terminate(_Reason, _Srv) ->
>> +terminate(Reason, _Srv) ->
>> +    couch_util:terminate_linked(Reason),
>>    ok.
>>
>>
>>
>> Modified: couchdb/trunk/src/couchdb/couch_server.erl
>> URL: http://svn.apache.org/viewvc/couchdb/trunk/src/couchdb/couch_server.erl?rev=763858&r1=763857&r2=763858&view=diff
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> =====================================================================
>> --- couchdb/trunk/src/couchdb/couch_server.erl (original)
>> +++ couchdb/trunk/src/couchdb/couch_server.erl Fri Apr 10 02:21:37  
>> 2009
>> @@ -159,7 +159,8 @@
>>                max_dbs_open=MaxDbsOpen,
>>                start_time=httpd_util:rfc1123_date()}}.
>>
>> -terminate(_Reason, _Server) ->
>> +terminate(Reason, _Srv) ->
>> +    couch_util:terminate_linked(Reason),
>>    ok.
>>
>> all_databases() ->
>>
>> Modified: couchdb/trunk/src/couchdb/couch_util.erl
>> URL: http://svn.apache.org/viewvc/couchdb/trunk/src/couchdb/couch_util.erl?rev=763858&r1=763857&r2=763858&view=diff
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> =====================================================================
>> --- couchdb/trunk/src/couchdb/couch_util.erl (original)
>> +++ couchdb/trunk/src/couchdb/couch_util.erl Fri Apr 10 02:21:37 2009
>> @@ -12,7 +12,7 @@
>>
>> -module(couch_util).
>>
>> --export([start_driver/1]).
>> +-export([start_driver/1,terminate_linked/1]).
>> -export([should_flush/0, should_flush/1, to_existing_atom/1,  
>> to_binary/1]).
>> -export([new_uuid/0, rand32/0, implode/2, collate/2, collate/3]).
>> -export([abs_pathname/1,abs_pathname/2, trim/1, ascii_lower/1]).
>> @@ -43,6 +43,14 @@
>>    V.
>>
>>
>> +terminate_linked(normal) ->
>> +    terminate_linked(shutdown);
>> +terminate_linked(Reason) ->
>> +    {links, Links} = process_info(self(), links),
>> +    [catch exit(Pid, Reason) || Pid <- Links],
>> +    ok.
>> +
>> +
>> new_uuid() ->
>>    list_to_binary(to_hex(crypto:rand_bytes(16))).
>>
>>
>> Modified: couchdb/trunk/src/couchdb/couch_view.erl
>> URL: http://svn.apache.org/viewvc/couchdb/trunk/src/couchdb/couch_view.erl?rev=763858&r1=763857&r2=763858&view=diff
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> =====================================================================
>> --- couchdb/trunk/src/couchdb/couch_view.erl (original)
>> +++ couchdb/trunk/src/couchdb/couch_view.erl Fri Apr 10 02:21:37 2009
>> @@ -226,7 +226,9 @@
>>    process_flag(trap_exit, true),
>>    {ok, #server{root_dir=RootDir}}.
>>
>> -terminate(_Reason,_State) ->
>> +
>> +terminate(Reason, _Srv) ->
>> +    couch_util:terminate_linked(Reason),
>>    ok.
>>
>>
>>
>> Modified: couchdb/trunk/src/couchdb/couch_view_group.erl
>> URL: http://svn.apache.org/viewvc/couchdb/trunk/src/couchdb/couch_view_group.erl?rev=763858&r1=763857&r2=763858&view=diff
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> =====================================================================
>> --- couchdb/trunk/src/couchdb/couch_view_group.erl (original)
>> +++ couchdb/trunk/src/couchdb/couch_view_group.erl Fri Apr 10  
>> 02:21:37 2009
>> @@ -272,6 +272,7 @@
>>
>> terminate(Reason, State) ->
>>    reply_all(State, Reason),
>> +    couch_util:terminate_linked(Reason),
>>    ok.
>>
>> code_change(_OldVsn, State, _Extra) ->
>>
>>
>


Mime
View raw message