[GitHub] [couchdb] nickva commented on a diff in pull request #4199: Fix spurious unlock in close_db_if_idle

[GitBox <gi...@apache.org>]

nickva commented on code in PR #4199:
URL: https://github.com/apache/couchdb/pull/4199#discussion_r990670277


##########
src/couch/src/couch_server.erl:
##########
@@ -31,6 +31,7 @@
 -export([num_servers/0, couch_server/1, couch_dbs_pid_to_name/1, couch_dbs/1]).
 -export([aggregate_queue_len/0, get_spidermonkey_version/0]).
 -export([names/0]).
+-export([try_lock/2]).

Review Comment:
   Should we have a symmetric `unlock/2` function as well. Just so locking/unlocking happens in those utility functions only?



##########
src/couch/src/couch_lru.erl:
##########
@@ -46,9 +46,8 @@ close_int({Lru, DbName, Iter}, {Tree, Dict} = Cache) ->
     CouchDbs = couch_server:couch_dbs(DbName),
     CouchDbsPidToName = couch_server:couch_dbs_pid_to_name(DbName),
 
-    case ets:update_element(CouchDbs, DbName, {#entry.lock, locked}) of
-        true ->
-            [#entry{db = Db, pid = Pid}] = ets:lookup(CouchDbs, DbName),
+    case couch_server:try_lock(DbName, CouchDbs) of

Review Comment:
   Argument order is reversed here CouchDbs is the table reference and DbName should be key.



##########
src/couch/src/couch_server.erl:
##########
@@ -1011,6 +1011,21 @@ names() ->
     N = couch_server:num_servers(),
     [couch_server:couch_server(I) || I <- lists:seq(1, N)].
 
+%% Try to lock an entry, must be unlocked at the time.
+try_lock(Table, DbName) ->

Review Comment:
   Might be a good idea to assert the types, that `Table` is an atom and `DbName` a binary?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org