You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by davisp <gi...@git.apache.org> on 2016/08/09 00:21:58 UTC
[GitHub] couchdb-config pull request #10: 3096 fix config listener accumulation
GitHub user davisp opened a pull request:
https://github.com/apache/couchdb-config/pull/10
3096 fix config listener accumulation
This fixes an issue we've found with event_handlers not being removed from config_event because they're registered in the config gen_server which never exits. This fixes the issue by creating a monitor process that maintains the current callback API that we have rather than reverting to the half callback/half message passing callback.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cloudant/couchdb-config 3096-fix-config-listener-accumulation
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/couchdb-config/pull/10.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #10
----
commit dbde991056c8f4c8ced898d7b7334638727f6be7
Author: Paul J. Davis <pa...@gmail.com>
Date: 2016-08-05T19:41:58Z
Fix config listener event handler registration
We rely on `gen_event:add_sup_handler/3` to remove handlers when the
process that registered for events exits. On master this was changed so
that config becomes the process that's monitored by gen_event. As such
any handler that is registered (say, for when an index is opened) adds a
handler to the config_event gen_event process. Since the config process
never exits these handlers are never removed.
The end result of all of this is that on a busy cluster the config_event
process will end up with millions of handlers consuming many gigabytes
of RAM.
This change creates a monitor process for every event handler. This
monitors the process wanting to listen for config changes and exits when
the requesting process exits. This means that we maintain our pure
callback API improvement while correctly removing handlers.
COUCHDB-3096
commit ab5518188a50420142b1903f3c1c3c27554c0587
Author: Paul J. Davis <pa...@gmail.com>
Date: 2016-08-05T19:49:17Z
Clean up config_tests
This cleans up the config_tests both stylistically as well as removes
some race conditions around message passing from the config handler.
This also reformats and changes a lot of the tests so that we're using
consistent patterns through out the file. Unfortunately foreach and
foreachx are terrible constructs and require using the `?_test/1` macro
which is a bit annoying but ended up being the least worst approach I
could find.
COUCHDB-3096
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config issue #10: 3096 fix config listener accumulation
Posted by davisp <gi...@git.apache.org>.
Github user davisp commented on the issue:
https://github.com/apache/couchdb-config/pull/10
Good catch on that function name. I'll change that to subscribe.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config issue #10: 3096 fix config listener accumulation
Posted by davisp <gi...@git.apache.org>.
Github user davisp commented on the issue:
https://github.com/apache/couchdb-config/pull/10
But wouldn't that completely undercut the entire change to avoid having config_event send a message to the process that calls `config:listen_for_changes/2`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config issue #10: 3096 fix config listener accumulation
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the issue:
https://github.com/apache/couchdb-config/pull/10
Wow! Nice found!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config issue #10: 3096 fix config listener accumulation
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the issue:
https://github.com/apache/couchdb-config/pull/10
`gen_event` establishes [a link](https://github.com/erlang/otp/blob/maint/lib/stdlib/src/gen_event.erl#L444) and [removes the handler](https://github.com/erlang/otp/blob/maint/lib/stdlib/src/gen_event.erl#L263) in [handle_exit](https://github.com/erlang/otp/blob/maint/lib/stdlib/src/gen_event.erl#L335:L353).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config issue #10: 3096 fix config listener accumulation
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the issue:
https://github.com/apache/couchdb-config/pull/10
@davisp: Other than a [little nitpick](https://github.com/apache/couchdb-config/pull/10/files#r74124401) (feel free to ignore if you disagree) it looks good. +1 from me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request #10: 3096 fix config listener accumulation
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/couchdb-config/pull/10
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config issue #10: 3096 fix config listener accumulation
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the issue:
https://github.com/apache/couchdb-config/pull/10
@davisp: Is there any reason why we cannot rely on gen_event provided monitoring. The following change passes the test suite:
```
--- a/src/config.erl
+++ b/src/config.erl
@@ -183,7 +183,7 @@ delete(Section, Key, Persist, Reason) when is_list(Section), is_list(Key) ->
listen_for_changes(CallbackModule, InitialState) ->
- config_listener_mon:start_link(CallbackModule, InitialState).
+ config_listener:start(CallbackModule, InitialState).
```
```
--- a/src/config_listener.erl
+++ b/src/config_listener.erl
@@ -35,7 +35,9 @@ start(Module, State) ->
start(Module, Module, State).
start(Module, Id, State) ->
- gen_event:add_sup_handler(config_event, {?MODULE, Id}, {Module, State}).
+ Pid = self(),
+ gen_event:add_sup_handler(config_event,
+ {?MODULE, {Id, Pid}}, {Module, {Pid, State}}).
```
```
--- a/test/config_tests.erl
+++ b/test/config_tests.erl
@@ -461,10 +461,6 @@ should_remove_handler_when_pid_exits(Pid) ->
?_test(begin
?assertEqual(2, n_handlers()),
- % Monitor the config_listener_mon process
- {monitored_by, [Mon]} = process_info(Pid, monitored_by),
- MonRef = erlang:monitor(process, Mon),
-
% Kill the process synchronously
PidRef = erlang:monitor(process, Pid),
exit(Pid, kill),
@@ -474,14 +470,6 @@ should_remove_handler_when_pid_exits(Pid) ->
erlang:error({timeout, config_listener_death})
end,
- % Wait for the config_listener_mon process to
- % exit to indicate the handler has been removed.
- receive
- {'DOWN', MonRef, _, _, _} -> ok
- after ?TIMEOUT ->
- erlang:error({timeout, config_listener_mon_death})
- end,
-
?assertEqual(1, n_handlers())
end).
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config issue #10: 3096 fix config listener accumulation
Posted by davisp <gi...@git.apache.org>.
Github user davisp commented on the issue:
https://github.com/apache/couchdb-config/pull/10
Oh, I guess config_listener_mon also uses a monitor back to the requesting process rather than a link. I did that because its slightly cleaner than trapping exits in the monitor as well as it doesn't trigger a message anywhere if it exits.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request #10: 3096 fix config listener accumulation
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on a diff in the pull request:
https://github.com/apache/couchdb-config/pull/10#discussion_r74083294
--- Diff: src/config_listener_mon.erl ---
@@ -0,0 +1,84 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(config_listener_mon).
+-behaviour(gen_server).
+-vsn(1).
+
+
+-export([
+ start_link/2
+]).
+
+
+-export([
+ init/1,
+ terminate/2,
+ handle_call/3,
+ handle_cast/2,
+ handle_info/2,
+ code_change/3
+]).
+
+
+-record(st, {
+ pid,
+ ref
+}).
+
+
+start_link(Module, InitSt) ->
+ proc_lib:start(?MODULE, init, [{self(), Module, InitSt}]).
+
+
+init({Pid, Mod, InitSt}) ->
+ Ref = erlang:monitor(process, Pid),
+ case config_listener:start(Mod, {Mod, Pid}, {Pid, InitSt}) of
--- End diff --
Currently we pass InitState to Module:handle_config_change. With this state we would start passing `{Pid, InitSt}` which has two problems:
- it breaks current contract
- it leaks internals
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config issue #10: 3096 fix config listener accumulation
Posted by davisp <gi...@git.apache.org>.
Github user davisp commented on the issue:
https://github.com/apache/couchdb-config/pull/10
@iilyak I changed the function to subscribe and added a test to show that gen_event_EXIT is handled properly. I'll wait for another +1 on that new test before merging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config issue #10: 3096 fix config listener accumulation
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the issue:
https://github.com/apache/couchdb-config/pull/10
I see. We want to isolate the caller from config_event. So the config_event's exit for example do not propagate to the process that calls config:listen_for_changes. So want to re-implement monitoring based on monitor rather on link.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request #10: 3096 fix config listener accumulation
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on a diff in the pull request:
https://github.com/apache/couchdb-config/pull/10#discussion_r74124401
--- Diff: src/config_listener_mon.erl ---
@@ -0,0 +1,84 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(config_listener_mon).
+-behaviour(gen_server).
+-vsn(1).
+
+
+-export([
+ start_link/2
+]).
+
+
+-export([
+ init/1,
+ terminate/2,
+ handle_call/3,
+ handle_cast/2,
+ handle_info/2,
+ code_change/3
+]).
+
+
+-record(st, {
+ pid,
+ ref
+}).
+
+
+start_link(Module, InitSt) ->
--- End diff --
A nitpick. This a little confusing. The function is called `start_link`. `start_link` usually returns `{ok, Pid}`. In this case we return `ok`. We could either:
- make it return `{ok, Pid}`
- rename it to something else (`subscribe` | `monitor` | `listen`)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request #10: 3096 fix config listener accumulation
Posted by davisp <gi...@git.apache.org>.
Github user davisp commented on a diff in the pull request:
https://github.com/apache/couchdb-config/pull/10#discussion_r74089266
--- Diff: src/config_listener_mon.erl ---
@@ -0,0 +1,84 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(config_listener_mon).
+-behaviour(gen_server).
+-vsn(1).
+
+
+-export([
+ start_link/2
+]).
+
+
+-export([
+ init/1,
+ terminate/2,
+ handle_call/3,
+ handle_cast/2,
+ handle_info/2,
+ code_change/3
+]).
+
+
+-record(st, {
+ pid,
+ ref
+}).
+
+
+start_link(Module, InitSt) ->
+ proc_lib:start(?MODULE, init, [{self(), Module, InitSt}]).
+
+
+init({Pid, Mod, InitSt}) ->
+ Ref = erlang:monitor(process, Pid),
+ case config_listener:start(Mod, {Mod, Pid}, {Pid, InitSt}) of
--- End diff --
I agree that it leaks internals but it doesn't change anything on the current behavior. I had to do this to match the old behavior where we were passing {Subscriber, InitState} with the gen_event handler so that the Subscriber pid could be passed to handle_config_terminate (weirdly). I just moved it along with the call to config_listener:start/3 rather than keeping it wrapped in config.erl's call to config_listener_mon:start_link/2.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request #10: 3096 fix config listener accumulation
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on a diff in the pull request:
https://github.com/apache/couchdb-config/pull/10#discussion_r74121606
--- Diff: src/config_listener_mon.erl ---
@@ -0,0 +1,84 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(config_listener_mon).
+-behaviour(gen_server).
+-vsn(1).
+
+
+-export([
+ start_link/2
+]).
+
+
+-export([
+ init/1,
+ terminate/2,
+ handle_call/3,
+ handle_cast/2,
+ handle_info/2,
+ code_change/3
+]).
+
+
+-record(st, {
+ pid,
+ ref
+}).
+
+
+start_link(Module, InitSt) ->
+ proc_lib:start(?MODULE, init, [{self(), Module, InitSt}]).
+
+
+init({Pid, Mod, InitSt}) ->
+ Ref = erlang:monitor(process, Pid),
+ case config_listener:start(Mod, {Mod, Pid}, {Pid, InitSt}) of
--- End diff --
I was wrong it wouldn't break the contract. Since we do pass Pid already
[Reply = config_listener:start(CallbackModule, {Subscriber, InitialState})](https://github.com/apache/couchdb-config/blob/master/src/config.erl#L264)
The config_listener would make sure Pid is not passed to [Module:handle_config_change](https://github.com/apache/couchdb-config/blob/master/src/config_listener.erl#L43:L44)
```
handle_event({config_change, Sec, Key, Value, Persist}, {Module, {From, State}}) ->
| |
x |
V
case Module:handle_config_change(Sec, Key, Value, Persist, State) of
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config issue #10: 3096 fix config listener accumulation
Posted by davisp <gi...@git.apache.org>.
Github user davisp commented on the issue:
https://github.com/apache/couchdb-config/pull/10
@iilyak I'm not sure what you mean by that last sentence. The previous change that introduced this issue was focused on removing the split between message passing and module callbacks. To do that we need a separate process. In the first case that was the config gen_event which turned out to be bad because it never dies so handlers are never removed. This just creates a new config_listener_mon that exits when the requesting process does as a way to trigger the handler removal.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config issue #10: 3096 fix config listener accumulation
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the issue:
https://github.com/apache/couchdb-config/pull/10
All tests pass locally. +1
```
module 'config'
config: to_integer_test...ok
config: to_float_test...ok
module 'config_tests'
Config get tests
config_tests: config_get_test_...ok
config_tests: config_get_test_...ok
config_tests: config_get_test_...ok
config_tests: config_get_test_...ok
config_tests: config_get_test_...ok
config_tests: config_get_test_...ok
config_tests: config_get_test_...ok
config_tests: config_get_test_...ok
config_tests: config_get_test_...ok
[done in 0.557 s]
Config set tests
config_tests: config_set_test_...[0.022 s] ok
config_tests: config_set_test_...[0.001 s] ok
config_tests: config_set_test_...ok
[done in 0.109 s]
Config deletion tests
config_tests: config_del_test_...ok
config_tests: config_del_test_...ok
[done in 0.052 s]
Configs overide tests
config_tests:321: should_ensure_in_defaults...ok
config_tests:329: should_override_options...ok
config_tests:354: should_create_new_sections_on_override...ok
config_tests:361: should_win_last_in_chain...ok
config_tests:336: should_read_default_d...ok
config_tests:342: should_read_local_d...ok
config_tests:348: should_read_default_and_local_d...ok
[done in 0.176 s]
Config persistent changes
config_tests:367: should_write_changes...[notice] config: [httpd] port set to 8080 for reason nil
[notice] config: [httpd] bind_address deleted for reason "8080"
[0.024 s] ok
config_tests:377: should_ensure_default_wasnt_modified...[0.001 s] ok
config_tests:384: should_ensure_written_to_last_config_in_chain...ok
[done in 0.095 s]
Test config with no files
config_tests: config_no_files_test_...ok
config_tests: config_no_files_test_...ok
config_tests: config_no_files_test_...ok
[done in 0.046 s]
Test config_listener behaviour
config_tests:409: should_handle_value_change...ok
config_tests:416: should_pass_correct_state_to_handle_config_change...ok
config_tests:425: should_pass_correct_state_to_handle_config_terminate...ok
config_tests:436: should_pass_subscriber_pid_to_handle_config_terminate...[0.001 s] ok
config_tests:443: should_not_call_handle_config_after_related_process_death...[0.255 s] ok
config_tests:456: should_remove_handler_when_requested...[0.001 s] ok
config_tests:465: should_remove_handler_when_pid_exits...[0.001 s] ok
config_tests:494: should_stop_monitor_on_error...[0.001 s] ok
[done in 0.497 s]
[done in 1.533 s]
[done in 1.539 s]
=======================================================
All 37 tests passed.
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config issue #10: 3096 fix config listener accumulation
Posted by davisp <gi...@git.apache.org>.
Github user davisp commented on the issue:
https://github.com/apache/couchdb-config/pull/10
I should mention that the end result of this bug is that handlers are never removed. On a busy cluster we were seeing tens of millions of handlers which led to config_event using ten to fifteen gigabytes of RAM which is mmmmbad.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---