Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix chttpd auth cache log errors #1950

Closed

Conversation

jaydoane
Copy link
Contributor

@jaydoane jaydoane commented Feb 27, 2019

Overview

In situations where the chttpd authenticaion db (usually named "_users")
does not get created, or gets deleted, the auth cache still attempts
to start the change listener, which results in notice and error level
logs in the system like:

[notice] 2018-12-03T22:04:44.299392Z node1@127.0.0.1 <0.374.0> -------- chttpd_auth_cache changes listener died database_does_not_exist at mem3_shards:load_shards_from_db/6(line:395) <= mem3_shards:load_shards_from_disk/1(line:370) <= mem3_shards:load_shards_from_disk/2(line:399) <= mem3_shards:for_docid/3(line:86) <= fabric_doc_open:go/3(line:39) <= chttpd_auth_cache:ensure_auth_ddoc_exists/2(line:195) <= chttpd_auth_cache:listen_for_changes/1(line:142)
[error] 2018-12-03T22:04:44.299585Z node1@127.0.0.1 emulator -------- Error in process <0.3180.0> on node 'node1@127.0.0.1' with exit value:
{database_does_not_exist,[{mem3_shards,load_shards_from_db,"_users",[{file,"src/mem3_shards.erl"},{line,395}]},{mem3_shards,load_shards_from_disk,1,[{file,"src/mem3_shards.erl"},{line,370}]},{mem3_shards,load_shards_from_disk,2,[{file,"src/mem3_shards.erl"},{line,399}]},{mem3_shards,for_docid,3,[{file,"src/mem3_shards.erl"},{line,86}]},{fabric_doc_open,go,3,[{file,"src/fabric_doc_open.erl"},{line,39}]},{chttpd_auth_cache,ensure_auth_ddoc_exists,2,[{file,"src/chttpd_auth_cache.erl"},{line,195}]},{chttpd_auth_cache,listen_for_changes,1,[{file,"src/chttpd_auth_cache.erl"},{line,142}]}]}

This changes the auth cache so that it

• Only starts change listener if auth db exists
• Only retains EndSeq if auth db still exists

Fixes #1354
Fixes #1949

Testing recommendations

make eunit apps=chttpd suites=chttpd_auth_cache_test

Related Issues or Pull Requests

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;

In situations where the chttpd authenticaion db (usually named "_users")
does not get created, or gets deleted, the auth cache still attempts
to start the change listener, which results in notice and error level
logs in the system like:

[notice] 2018-12-03T22:04:44.299392Z node1@127.0.0.1 <0.374.0> -------- chttpd_auth_cache changes listener died database_does_not_exist at mem3_shards:load_shards_from_db/6(line:395) <= mem3_shards:load_shards_from_disk/1(line:370) <= mem3_shards:load_shards_from_disk/2(line:399) <= mem3_shards:for_docid/3(line:86) <= fabric_doc_open:go/3(line:39) <= chttpd_auth_cache:ensure_auth_ddoc_exists/2(line:195) <= chttpd_auth_cache:listen_for_changes/1(line:142)

[error] 2018-12-03T22:04:44.299585Z node1@127.0.0.1 emulator -------- Error in process <0.3180.0> on node 'node1@127.0.0.1' with exit value:
{database_does_not_exist,[{mem3_shards,load_shards_from_db,"_users",[{file,"src/mem3_shards.erl"},{line,395}]},{mem3_shards,load_shards_from_disk,1,[{file,"src/mem3_shards.erl"},{line,370}]},{mem3_shards,load_shards_from_disk,2,[{file,"src/mem3_shards.erl"},{line,399}]},{mem3_shards,for_docid,3,[{file,"src/mem3_shards.erl"},{line,86}]},{fabric_doc_open,go,3,[{file,"src/fabric_doc_open.erl"},{line,39}]},{chttpd_auth_cache,ensure_auth_ddoc_exists,2,[{file,"src/chttpd_auth_cache.erl"},{line,195}]},{chttpd_auth_cache,listen_for_changes,1,[{file,"src/chttpd_auth_cache.erl"},{line,142}]}]}

This changes the auth cache so that it

• Only starts change listener if auth db exists
• Only retains EndSeq if auth db still exists

Fixes apache#1354
Fixes apache#1949
@jaydoane jaydoane force-pushed the fix-chttpd_auth_cache-log-errors branch from 0b490f3 to 8f2f436 Compare March 2, 2019 18:45
@jaydoane jaydoane marked this pull request as ready for review March 2, 2019 18:49
@wohali
Copy link
Member

wohali commented Mar 2, 2019

@jaydoane One unintended side effect of this is that there is no logged warning that the system databases are missing anymore. We should log an error once at least.

@janl
Copy link
Member

janl commented Mar 4, 2019

+1 for at least one log


change_listener_should_not_start_if_auth_db_missing(_DbName) ->
?_test(begin
%% ok = restart_app(chttpd),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commented line


change_listener_should_start_if_auth_db_exists(DbName) ->
?_test(begin
config:set("chttpd_auth", "authentication_db", ?b2l(DbName), false),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

config updates need to be moved into setup/teardown. Otherwise we might have the config set for subsequent tests when the current one fails.

foreach,
fun setup/0, fun teardown/1,
[
fun change_listener_should_not_start_if_auth_db_missing/1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are not covering all possible scenarios:

  • change_listener_starts_after_auth_db_created
  • change_listener_stops_and_not_restarted_after_auth_db_deleted

handle_info({start_listener, Seq}, State) ->
{noreply, State#state{changes_pid = spawn_changes(Seq)}};
erlang:send_after(5000, self(), {maybe_start_listener, Seq}),
{noreply, State#state{last_seq = Seq, changes_pid = undefined}};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch with missing changes_pid = undefined in previous version of the code.

@wohali
Copy link
Member

wohali commented Oct 9, 2020

@jaydoane any plans to get this in? I am preparing to mass-close old PRs that never got merged.

@jaydoane
Copy link
Contributor Author

@jaydoane any plans to get this in? I am preparing to mass-close old PRs that never got merged.

@wohali oh no, I completely forgot about this one 🙀

It seems like the main issues left are including more test cases, and adding some logging for missing _users db? If there's general agreement that this is a desirable PR, I'm happy to make those changes and get it finished.

@wohali
Copy link
Member

wohali commented Oct 12, 2020

Yes please.

@wohali wohali changed the base branch from master to main October 21, 2020 18:19
@jaydoane jaydoane force-pushed the fix-chttpd_auth_cache-log-errors branch from 2e70e6e to 8f2f436 Compare October 26, 2020 04:05
@jaydoane jaydoane changed the base branch from main to 3.x December 14, 2020 21:40
@jaydoane jaydoane changed the base branch from 3.x to main December 14, 2020 21:40
@jaydoane
Copy link
Contributor Author

jaydoane commented Jan 5, 2021

I took another look at this, but noticed this commit from Jan already addressed the issue of stack traces in the logs, and the repeating error does actually seem like a good -- albeit somewhat spammy -- feature. So I think we should just close this in favor of his more elegant fix.

@jaydoane jaydoane closed this Jan 5, 2021
@jaydoane jaydoane deleted the fix-chttpd_auth_cache-log-errors branch January 8, 2021 04:24
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
4 participants