You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2022/10/07 11:10:13 UTC
[GitHub] [couchdb] rnewson opened a new pull request, #4199: Fix spurious unlock in close_db_if_idle
rnewson opened a new pull request, #4199:
URL: https://github.com/apache/couchdb/pull/4199
We've observed `couch_db:incref/1` being called with `undefined` under load. On examination of couch_server it seems we unlock an `#entry` unilaterally, which can leave an `#entry` where lock is `unlocked` but `db` is not yet populated, and hence the value based to `incref` is the default for the record, which is `undefined`.
This patch modifies close_db_if_idle to only proceed if it finds an unlocked #entry.
--
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
[GitHub] [couchdb] nickva commented on a diff in pull request #4199: Fix spurious unlock in close_db_if_idle
Posted by GitBox <gi...@apache.org>.
nickva commented on code in PR #4199:
URL: https://github.com/apache/couchdb/pull/4199#discussion_r991421644
##########
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:
The main idea would to keep the logic of locking/unlocking is in one place, so if we see the `try_lock/2` in `couch_server` the function right bellow would be `unlock/2` so it's obvious how unlocking happens. It's just one line but the semantics of it is pretty tricky to reason about (like we noticed it was spread between two separate modules).
--
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
[GitHub] [couchdb] rnewson commented on a diff in pull request #4199: Fix spurious unlock in close_db_if_idle
Posted by GitBox <gi...@apache.org>.
rnewson commented on code in PR #4199:
URL: https://github.com/apache/couchdb/pull/4199#discussion_r991084710
##########
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:
I thought about it but it wouldn't be "symmetric" (in the sense that try_unlock would only unlock if locked, using another select_replace). We _know_ we acquired the lock, so we can release it without checking. Is that worth pulling out into an `unlock` function which is just the `update_element` function? I could do it, not sure it clarifies much. thoughts?
##########
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:
yeah..
--
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
[GitHub] [couchdb] rnewson commented on a diff in pull request #4199: Fix spurious unlock in close_db_if_idle
Posted by GitBox <gi...@apache.org>.
rnewson commented on code in PR #4199:
URL: https://github.com/apache/couchdb/pull/4199#discussion_r991433983
##########
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:
sure.
--
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
[GitHub] [couchdb] rnewson merged pull request #4199: Fix spurious unlock in close_db_if_idle
Posted by GitBox <gi...@apache.org>.
rnewson merged PR #4199:
URL: https://github.com/apache/couchdb/pull/4199
--
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
[GitHub] [couchdb] nickva commented on a diff in pull request #4199: Fix spurious unlock in close_db_if_idle
Posted by 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
[GitHub] [couchdb] nickva commented on a diff in pull request #4199: Fix spurious unlock in close_db_if_idle
Posted by GitBox <gi...@apache.org>.
nickva commented on code in PR #4199:
URL: https://github.com/apache/couchdb/pull/4199#discussion_r990669151
##########
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 the key.
--
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
[GitHub] [couchdb] rnewson commented on a diff in pull request #4199: Fix spurious unlock in close_db_if_idle
Posted by GitBox <gi...@apache.org>.
rnewson commented on code in PR #4199:
URL: https://github.com/apache/couchdb/pull/4199#discussion_r991084969
##########
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:
have done this, good thought.
--
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