You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by iilyak <gi...@git.apache.org> on 2016/08/16 23:39:40 UTC
[GitHub] couchdb-config pull request #11: Move responsibility to re-add handler to co...
GitHub user iilyak opened a pull request:
https://github.com/apache/couchdb-config/pull/11
Move responsibility to re-add handler to config
We move responsibility to re-add handler from config_listener behaviour
to config_listener_mon module.
gen_event behaviour is a tricky one. Due to its prehistoric times it
uses links instead of monitors (which didn't exist at that time). This
makes dealing with gen_event manager restart tricky.
We use {swap_handler, Reason, OldState, NewHandler, NewState} reply from
handle_event callback to be able to catch any exception from
Mod:handle_config_change. Effectively preventing gen_event manager from
killing config_listener_mon process. This trick allows us:
- detect all kinds of failures and re-add handler
- detect crash of gen_event manager to restart config_listener_mon
As a result of this change the config_listener behaviour modules need to
be updated to remove following code:
handle_config_terminate(_Server, _Reason, State) ->
spawn(fun() ->
timer:sleep(5000),
config:listen_for_changes(?MODULE, State)
end).
The handler would be re-added automatically. You can disable this
behaviour by returning `remove_handler` atom from handle_config_terminate/3
COUCHDB-3102
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cloudant/couchdb-config 3102-restart-monitor
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/couchdb-config/pull/11.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 #11
----
commit 7402d78539bca11d99e4cd9f29f25263f4107fe6
Author: ILYA Khlopotov <ii...@ca.ibm.com>
Date: 2016-08-15T23:55:09Z
Consider only needed handlers in n_handlers
When testing. Make sure we don't count other config_event handlers
present in the system.
commit e936d6a470d5bcec880c474c00f9bf935c42d404
Author: ILYA Khlopotov <ii...@ca.ibm.com>
Date: 2016-08-16T22:56:34Z
Fix compilation warning
commit 99f38142d5446206f4878e895648304ff8303642
Author: ILYA Khlopotov <ii...@ca.ibm.com>
Date: 2016-08-16T20:52:12Z
Move responsibility to re-add handler to config
We move responsibility to re-add handler from config_listener behaviour
to config_listener_mon module.
gen_event behaviour is a tricky one. Due to its prehistoric times it
uses links instead of monitors (which didn't exist at that time). This
makes dealing with gen_event manager restart tricky.
We use {swap_handler, Reason, OldState, NewHandler, NewState} reply from
handle_event callback to be able to catch any exception from
Mod:handle_config_change. Effectively preventing gen_event manager from
killing config_listener_mon process. This trick allows us:
- detect all kinds of failures and re-add handler
- detect crash of gen_event manager to restart config_listener_mon
As a result of this change the config_listener behaviour modules need to
be updated to remove following code:
handle_config_terminate(_Server, _Reason, State) ->
spawn(fun() ->
timer:sleep(5000),
config:listen_for_changes(?MODULE, State)
end).
The handler would be re-added automatically. You can disable this
behaviour by returning `remove_handler` atom from handle_config_terminate/3
COUCHDB-3102
----
---
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 #11: Move responsibility to re-add handler to config
Posted by davisp <gi...@git.apache.org>.
Github user davisp commented on the issue:
https://github.com/apache/couchdb-config/pull/11
+1
---
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 #11: Move responsibility to re-add handler to config
Posted by davisp <gi...@git.apache.org>.
Github user davisp commented on the issue:
https://github.com/apache/couchdb-config/pull/11
So I've been reading this and re-reading gen_event docs and so on and I have to say this seems to be fairly complicated for what its doing. I agree that moving the re-listen logic back to config_listener_mon makes sense but the whole swap_handler logic is quite lost on me. Can you explain how/why that works as it does?
---
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 #11: Move responsibility to re-add handler to co...
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/11#discussion_r75489766
--- Diff: src/config_notifier.erl ---
@@ -0,0 +1,76 @@
+% 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_notifier).
+
+-behaviour(gen_event).
+-vsn(1).
+
+%% Public interface
+-export([subscribe/1]).
+-export([subscribe/2]).
+
+-export([behaviour_info/1]).
+
+%% gen_event interface
+-export([
+ init/1,
+ handle_event/2,
+ handle_call/2,
+ handle_info/2,
+ terminate/2,
+ code_change/3
+]).
+
+behaviour_info(callbacks) ->
--- End diff --
This can go right?
---
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 #11: Move responsibility to re-add handler to co...
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/11#discussion_r75041083
--- Diff: test/config_tests.erl ---
@@ -572,4 +694,46 @@ getmsg(Pid) ->
n_handlers() ->
- length(gen_event:which_handlers(config_event)).
+ length(handlers()).
+
+handlers() ->
+ Handlers = gen_event:which_handlers(config_event),
+ [Pid || {config_listener, {?MODULE, Pid}} <- Handlers].
+
+%% copy/paste from test_util
--- End diff --
I am not sure if we can just reuse the functions from test_util. I noticed that @davisp didn't use functions from that module. So I decided to copy/paste. Let me know if we can reuse functions from test_util.
---
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 #11: Move responsibility to re-add handler to config
Posted by davisp <gi...@git.apache.org>.
Github user davisp commented on the issue:
https://github.com/apache/couchdb-config/pull/11
Pretty sure I +1'ed all the linked PRs as well after reviewing. If I missed any let me know as it was an accident.
---
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 #11: Move responsibility to re-add handler to co...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/couchdb-config/pull/11
---
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 #11: Move responsibility to re-add handler to config
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the issue:
https://github.com/apache/couchdb-config/pull/11
> @davisp: he only thing I see is that we should change config_listener_mon to use proc_lib:start_link and change the return value to {ok, Pid}
- `config_listener_mon` uses `proc_lib:start_link` already
- return `{ok, Pid}` - FIXED
Also I did rebase to squash fixup commits
---
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 #11: Move responsibility to re-add handler to config
Posted by davisp <gi...@git.apache.org>.
Github user davisp commented on the issue:
https://github.com/apache/couchdb-config/pull/11
Awesome. Much simpler. The only thing I see is that we should change config_listener_mon to use proc_lib:start_link and change the return value to {ok, Pid} and then when we go to update all of the other places we can configure all of the listeners using the supervision tree using that module directly. I.e., we can remove the gen_server from couch_log_config_listener.erl.
---
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 #11: Move responsibility to re-add handler to co...
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/11#discussion_r75137968
--- Diff: test/config_tests.erl ---
@@ -572,4 +694,46 @@ getmsg(Pid) ->
n_handlers() ->
- length(gen_event:which_handlers(config_event)).
+ length(handlers()).
+
+handlers() ->
+ Handlers = gen_event:which_handlers(config_event),
+ [Pid || {config_listener, {?MODULE, Pid}} <- Handlers].
+
+%% copy/paste from test_util
--- End diff --
That last bit we can obviously punt on for now and then just work to fix over time.
---
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 #11: Move responsibility to re-add handler to config
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the issue:
https://github.com/apache/couchdb-config/pull/11
One of the most important requirements of this design is to keep the caller's state.
We need state in order to re-add the listener when needed.
Let's go through failure scenarios for a our current implemenetation:
# Current implementation
## Mod:handle_event throws an exception
1. gen_event uses `catch` to [detect the failure](https://github.com/erlang/otp/blob/maint/lib/stdlib/src/gen_event.erl#L533)
2. then [it terminates](https://github.com/erlang/otp/blob/maint/lib/stdlib/src/gen_event.erl#L545) the handler
3. the termination logic [calls Mod:terminate](https://github.com/erlang/otp/blob/maint/lib/stdlib/src/gen_event.erl#L665) and sends [gen_event_EXIT](https://github.com/erlang/otp/blob/maint/lib/stdlib/src/gen_event.erl#L681)
We have the state passed to Mod:terminate so our invariant holds
## gen_event manager crashes (config_event process)
We receive {gen_event_EXIT, Handler, Reason} message then crash since we don't trap_exit
Unfortunatelly state is not passed back in the gen_event_EXIT. Our invariant is violated.
# The things I've tried:
## use config_listener_mon to track state
This didn't work well for two reasons:
1. Mod:handle_event has to call gen_server:call (it is a problem since we use sync_notify)
2. config_listener_mon crashes after config_event since we are not traping exits
## use config_listener_mon to track state and trap exits
This didn't work for following reasons:
1. Mod:handle_event has to call gen_server:call (it is a problem since we use sync_notify)
2. When config_listener_mon die we lose state
## Maintain two copies of state in config_listener and config_listener_mon
This approach didn't work since there is a case when we updated the state in config_listener_mon
and then crash. In this case config_listener would have older state.
# How new implementation work
We catch exceptions from `Mod:handle_event` ourselves to detect the case when handler throws an exception. In this case we return swap_handler with the same handler and state:
{swap_handler, {remove_handler_due_to_error, {error, Error}},
St, {config_listener, {Mod, Pid}}, St}
It works as follows:
1. `gen_event` [detects swap_handler result](https://github.com/erlang/otp/blob/maint/lib/stdlib/src/gen_event.erl#L538:L539)
2. We use [Args1](https://github.com/erlang/otp/blob/maint/lib/stdlib/src/gen_event.erl#L538) to pass reason as [{remove_handler_due_to_error, {error, Error}}](https://github.com/apache/couchdb-config/pull/11/files#diff-e603d3168cf3d6b71247b3564c1c21ecR59)
3. Args1 get passed to [Mod:terminate(Args1, State)](https://github.com/erlang/otp/blob/maint/lib/stdlib/src/gen_event.erl#L665)
4. We have reason and state so we can call `Mod:handle_config_terminate`
5. `gen_event` uses [{swapped, Handler2, SupervisedPid}](https://github.com/erlang/otp/blob/maint/lib/stdlib/src/gen_event.erl#L554) as a reason for termination. Which is documented in `gen_event` under `add_sup_handler` function.
6. `config_listener_mon` receives `gen_event_EXIT` sent [from here](https://github.com/erlang/otp/blob/maint/lib/stdlib/src/gen_event.erl#L681) and [ignores it](https://github.com/apache/couchdb-config/pull/11/files#diff-e603d3168cf3d6b71247b3564c1c21ecR99)
7. Then `gen_event` call [Mod2:init({Args2, State2})](https://github.com/erlang/otp/blob/maint/lib/stdlib/src/gen_event.erl#L556). As you can see it passes `Args2` (we have `#st{}` in there). The `Mod2:init({Args2, State2}` part is also documented in `gen_event` under `swap_handler` function.
The approach doesn't rely on any non-documented behaviours or implementation details.
The are three key ideas for the trick:
1. we `swap_handler` with itself (using same Id)
2. we use `Args1` of the `swap_handler` tuple to pass reason for termination
3. we ignore `{gen_event_EXIT, {swapped, ...}}` due to `#1`
---
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 #11: Move responsibility to re-add handler to config
Posted by davisp <gi...@git.apache.org>.
Github user davisp commented on the issue:
https://github.com/apache/couchdb-config/pull/11
Aha! I hadn't considered tracking the handler state and how that gets lost when we have a crash in the gen_event process itself.
However, the solution around solving that issue seems overly complicated to me. Reading through it I get the feeling we're trying pound a square peg into a round hole. It seems to me that it'd be much easier to insert all of the config handlers into the supervision tree so that we get all of the restart logic automatically. However as we've mentioned before that hurts us for the cases where the change handler isn't just sending a message or doing some other roughly stateless operation.
Out of curiosity though I've gone through every use of config listeners and categorized them whether they'd be supervisor compatible or not:
Supervisor Compatible:
src/chttpd/src/chttpd_config_listener.erl
src/couch/src/couch_auth_cache.erl
src/couch/src/couch_compaction_daemon.erl
src/couch/src/couch_external_manager.erl
src/couch/src/couch_httpd_vhost.erl
src/couch/src/couch_os_daemons.erl
src/couch/src/couch_proc_manager.erl
src/couch/src/couch_server.erl
src/couch/src/couch_sup.erl
src/couch/src/couch_uuids.erl
src/couch_event/src/couch_event_os_sup.erl
src/couch_index/src/couch_index_server.erl % Lack of init might be an issue
src/couch_log/src/couch_log_config_listener.erl
src/couch_peruser/src/couch_peruser.erl % Petty sure...
src/couch_replicator/src/couch_replicator_manager.erl
src/global_changes/src/global_changes_config_listener.erl
src/ioq/src/ioq.erl
src/mem3/src/mem3_shards.erl
src/mem3/src/mem3_sync_event_listener.erl
Not Compatible:
src/couch/src/couch_external_server.erl % Broken anyway
src/couch_index/src/couch_index.erl % Also broken
Of all of our config listeners the only two that are process dependent are for couch_external_server and couch_index. Almost everything else is just sending messages back to a registered process. The only weird one is couch_peruser because it doesn't seem to register itself and I can't figure out how its ever called. However that seems easily fixed from a quick skim.
For the two not compatible uses I'd add a config_notifier which is an gen_event handler but just takes a pid (and maybe a section or section/key to limit notifications) and then on config change just sends a message. In this case we'd also just use the link so if the config_listener (assuming we implement config_notifier via config_listener) dies then we kill the process that was wanting change notifications and rely on the "let it crash" aspects of the index and/or external_server to have things get restarted correctly.
---
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 #11: Move responsibility to re-add handler to co...
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/11#discussion_r75137891
--- Diff: test/config_tests.erl ---
@@ -572,4 +694,46 @@ getmsg(Pid) ->
n_handlers() ->
- length(gen_event:which_handlers(config_event)).
+ length(handlers()).
+
+handlers() ->
+ Handlers = gen_event:which_handlers(config_event),
+ [Pid || {config_listener, {?MODULE, Pid}} <- Handlers].
+
+%% copy/paste from test_util
--- End diff --
Not in this repo. There's on going work to make this more independent from the rest of couch so that other projects can use it without requiring all of couch.
The real solution would be to move all of our test boilerplate code to the couch_tests repo and then make it a test dep though.
---
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.
---