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 2021/02/08 14:00:10 UTC

[GitHub] [couchdb] janosgyerman opened a new pull request #3363: Extend error handling with an unhandled case

janosgyerman opened a new pull request #3363:
URL: https://github.com/apache/couchdb/pull/3363


   ## Overview
   
   Instead of returning the stack trace in HTTP response we would want to log it and return a `{"ref": stack_hash(Stack)}`.
   It is implemented already, but there are an unhandled edge case to get 'ERROR', fixed in chttpd.
   
   ## Testing recommendations
   
   I can reproduce to get some leaking information before the fix, with the following steps:
   
   Create an empty database and add 2 new documents
   To get an error, just delete some data from shard files.
   
   When I deleted just a few bytes (few hundreds of bytes) was not enough to stable get an error, the system seems to be so robust in case of filesystem based corruptions. 
   
   I used this small script to truncate shard files:
   ```
   % cat trunc.sh
   #!/bin/bash
   
   for i in `find ./dev/lib/ | grep couch | grep testdb`; do
   	wc -c $i
   	truncate -s 1000 $i
   done
   ```
   
   Run the script, keeps only first 1000 bytes of every related shard files.
   
   After truncate shards, I send a simple query:
   
   ```
   % curl -u adm:pass http://127.0.0.1:15984/testdb/_all_docs | jq
   {
     "error": "badmatch",
     "reason": "{'EXIT',{{badmatch,eof},\n         [{couch_file,read_raw_iolist_int,3,\n
                       [{file,\"src/couch_file.erl\"},{line,714}]},\n          {couch_file,handle_call,3,
                       [{file,\"src/couch_file.erl\"},{line,493}]},\n          {gen_server,try_handle_call,4
                       [{file,\"gen_server.erl\"},{line,661}]},\n          {gen_server,handle_msg,6,
                       [{file,\"gen_server.erl\"},{line,690}]},\n          {proc_lib,init_p_do_apply,3,
                       [{file,\"proc_lib.erl\"},{line,249}]}]}}",
     "ref": 774065757
   }
   ```
   
   After the fix the response should looks like this:
   
   ```
   % curl -u adm:pass http://127.0.0.1:15984/testdb/_all_docs | jq
   {
     "error": "unknown_error",
     "reason": "eof",
     "ref": 3189254621
   }
   ```
   
   
   ## Related Issues or Pull Requests
   
   https://github.ibm.com/cloudant/fdb-dbcore/issues/607
   
   ## Checklist
   
   - [x] Code is written and works correctly
   - [ ] Changes are covered by tests
   - [ ] Any new configurable parameters are documented in `rel/overlay/etc/default.ini`
   - [ ] A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation
   


----------------------------------------------------------------
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.

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

[GitHub] [couchdb] iilyak commented on pull request #3363: Remove stack traces from HTTP responses

Posted by GitBox <gi...@apache.org>.
iilyak commented on pull request #3363:
URL: https://github.com/apache/couchdb/pull/3363#issuecomment-888203471


   Not reproducible on main


-- 
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] iilyak commented on pull request #3363: Remove stack traces from HTTP responses

Posted by GitBox <gi...@apache.org>.
iilyak commented on pull request #3363:
URL: https://github.com/apache/couchdb/pull/3363#issuecomment-810852468


   I was unable to reproduce the issue on main without the patch. Using database corruption method.
   
   ```
   rr(couch_db).
   Options = [{user_ctx, #user_ctx{roles = [<<"admin">>]}}, {interactive, true}].
   {ok, #{db_prefix := Prefix, tx := Tx} = Db} = fabric2_db:create(<<"pr-3363">>, Options).
   fabric2_fdb:transactional(Db, fun(#{tx := Tx}) ->  erlfdb:clear_range_startswith(Tx, Prefix) end).
   ```
   
   ```
   curl -s -u adm:pass http://127.0.0.1:15984/pr-3363/_all_docs | jq '.'
   {
     "error": "unknown_error",
     "reason": "badarg",
     "ref": 1157647193
   }
   ```


-- 
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.

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

[GitHub] [couchdb] iilyak commented on pull request #3363: Remove stack traces from HTTP responses

Posted by GitBox <gi...@apache.org>.
iilyak commented on pull request #3363:
URL: https://github.com/apache/couchdb/pull/3363#issuecomment-811921994


   I also tried a different method of reproducing and also unable to trigger the stack trace in the response.
   
   ```
   ~/dev/couchdb on  main [$?]
   ❯ curl --user adm:pass -X PUT http://127.0.0.1:15984/pr-3366
   {"ok":true}
   
   ~/dev/couchdb on  main [$?]
   ❯ curl --user adm:pass -X GET http://127.0.0.1:15984/pr-3366/_all_docs
   {"total_rows":0,"offset":null,"rows":[
   
   ]}
   
   ~/dev/couchdb on  main [$?]
   ❯ fdbcli --exec 'writemode on; clearrange "" \xFF'
   >>> writemode on
   >>> clearrange  \xff
   Committed (9545526301752)
   
   ~/dev/couchdb on  main [$?]
   ❯ curl --user adm:pass -X GET http://127.0.0.1:15984/pr-3366/_all_docs
   {"error":"unknown_error","reason":"badarg","ref":2541622152}
   ```


-- 
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.

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

[GitHub] [couchdb] nickva edited a comment on pull request #3363: Remove stack traces from HTTP responses

Posted by GitBox <gi...@apache.org>.
nickva edited a comment on pull request #3363:
URL: https://github.com/apache/couchdb/pull/3363#issuecomment-776124292


   @janosgyerman
   
   Very nice. It's good to have friendly error message and send stack traces to the logs. 
   
   I wonder if on `main` the specific `EXIT` clause error won't even apply. We'd have to call a `gen_server` from the request process and then the callback inside would throw an `error` and I couldn't remember if there are any calls to gen_servers made from the request process.
   
   Also, what do the logs show with and without your fix on 3.x, would you mind sharing those? And then perhaps move the PR to the 3.x branch.
   
   On main we'd have to see if these types of stacktraces can be generated at all. Maybe we'd need to other clauses here not just 'EXIT'.  As a fun experiment to help generate error you could insert various `error(some_message)` or `exit(exit_message)` expression in lower level functions which read or write to FDB (something in fabric2_db, fabric2_fdb, erlfdb, maybe). To make it fancy you could make them a bit random and say something like:  `case random:uniform() < 0.1 of true -> exit(boom); false -> ok end.` so only 10% of the time you'd get the error.
   
   


----------------------------------------------------------------
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.

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

[GitHub] [couchdb] wohali commented on pull request #3363: Remove stack traces from HTTP responses

Posted by GitBox <gi...@apache.org>.
wohali commented on pull request #3363:
URL: https://github.com/apache/couchdb/pull/3363#issuecomment-799550136


   edit for posterity: this is the page to bookmark: https://ci-couchdb.apache.org/blue/organizations/jenkins/jenkin
   s-cm1%2FPullRequests/activity


----------------------------------------------------------------
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.

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

[GitHub] [couchdb] nickva commented on pull request #3363: Remove stack traces from HTTP responses

Posted by GitBox <gi...@apache.org>.
nickva commented on pull request #3363:
URL: https://github.com/apache/couchdb/pull/3363#issuecomment-776124292


   @janosgyerman
   
   Very nice. It's good to have friendly error message and send stack traces to the logs. 
   
   I wonder if on `main` the specific `EXIT` clause error won't even apply. We'd have to call a `gen_server` from the request process and then the callback inside would throw an `error` and I couldn't remember if there are any calls to gen_servers made from the request process.
   
   Also, what do the logs show with and without your fix on 3.x, would you mind sharing those? And then perhaps move the PR to the 3.x branch.
   
   On main we'd have to see if these types of stacktraces can be generated at all. Maybe we'd need to other clauses here not just `'EXIT'.  As a fun experiment to help generate error you could insert various `error(some_message)` or `exit(exit_message)` expression in lower level functions which read or write to FDB (something in fabric2_db, fabric2_fdb, erlfdb, maybe). To make it fancy you could make them a bit random and say something like:  `case random:uniform() < 0.1 of true -> exit(boom); false -> ok end.` so only 10% of the time you'd get the error.
   
   


----------------------------------------------------------------
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.

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

[GitHub] [couchdb] janl commented on pull request #3363: Remove stack traces from HTTP responses

Posted by GitBox <gi...@apache.org>.
janl commented on pull request #3363:
URL: https://github.com/apache/couchdb/pull/3363#issuecomment-799536264


   @wohali yeah, tried that too and didn’t see a restart button where I would normally expect it. I don’t think blue ocean solves my UX trouble ;)
   


----------------------------------------------------------------
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.

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

[GitHub] [couchdb] wohali edited a comment on pull request #3363: Remove stack traces from HTTP responses

Posted by GitBox <gi...@apache.org>.
wohali edited a comment on pull request #3363:
URL: https://github.com/apache/couchdb/pull/3363#issuecomment-799550136


   edit for posterity: this is the page to bookmark: https://ci-couchdb.apache.org/blue/organizations/jenkins/jenkins-cm1%2FPullRequests/activity


----------------------------------------------------------------
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.

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

[GitHub] [couchdb] wohali commented on pull request #3363: Remove stack traces from HTTP responses

Posted by GitBox <gi...@apache.org>.
wohali commented on pull request #3363:
URL: https://github.com/apache/couchdb/pull/3363#issuecomment-799532486


   @janl always click on the "Open Blue Ocean" button after clicking on "Details" on any PR, or bookmark this page: https://ci-couchdb.apache.org/blue/organizations/jenkins/pipelines/
   
   Way better UX.


----------------------------------------------------------------
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.

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

[GitHub] [couchdb] wohali commented on pull request #3363: Remove stack traces from HTTP responses

Posted by GitBox <gi...@apache.org>.
wohali commented on pull request #3363:
URL: https://github.com/apache/couchdb/pull/3363#issuecomment-798970517


   @janl log into jenkins @ ci-couchdb.apache.org with your Apache creds and press the restart icon (looks like a circular arrow) for the job on the branch


----------------------------------------------------------------
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.

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

[GitHub] [couchdb] rnewson commented on a change in pull request #3363: Remove stack traces from HTTP responses

Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #3363:
URL: https://github.com/apache/couchdb/pull/3363#discussion_r573183159



##########
File path: src/chttpd/src/chttpd.erl
##########
@@ -1036,6 +1036,8 @@ error_info({service_unavailable, Reason}) ->
     {503, <<"service unavailable">>, Reason};
 error_info({timeout, _Reason}) ->
     error_info(timeout);
+error_info({'EXIT', {Error, Stack}}) ->
+    error_info({Error, nil, Stack});

Review comment:
       this appears to just invoke a clause further down, which calls another. I think it better to shorten the path here. I think `error_info(Error)` ends up doing the same thing but calling error_info one further time not two?




----------------------------------------------------------------
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.

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

[GitHub] [couchdb] bessbd commented on pull request #3363: Remove stack traces from HTTP responses

Posted by GitBox <gi...@apache.org>.
bessbd commented on pull request #3363:
URL: https://github.com/apache/couchdb/pull/3363#issuecomment-779391955


   Thank you for this PR, @janosgyerman !
   Please mind that `main` operates with an FDB backend instead of shard files that are referred to in the PR description.


----------------------------------------------------------------
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.

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

[GitHub] [couchdb] janl commented on pull request #3363: Remove stack traces from HTTP responses

Posted by GitBox <gi...@apache.org>.
janl commented on pull request #3363:
URL: https://github.com/apache/couchdb/pull/3363#issuecomment-798959890


   the ci errors here were intermitted, how do we re-run the cloudbees/error check?


----------------------------------------------------------------
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.

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

[GitHub] [couchdb] janl commented on pull request #3363: Remove stack traces from HTTP responses

Posted by GitBox <gi...@apache.org>.
janl commented on pull request #3363:
URL: https://github.com/apache/couchdb/pull/3363#issuecomment-799528861


   @wohali thanks, I couldn’t for the life of me find the right page where the restart button shows, but it seems to be here
   
   https://ci-couchdb.apache.org/job/jenkins-cm1/job/PullRequests/job/PR-3363/16/


----------------------------------------------------------------
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.

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

[GitHub] [couchdb] iilyak commented on pull request #3363: Remove stack traces from HTTP responses

Posted by GitBox <gi...@apache.org>.
iilyak commented on pull request #3363:
URL: https://github.com/apache/couchdb/pull/3363#issuecomment-804843277


   @janosgyerman I remember you had a similar PR for 3.x branch but I cannot find. If we cannot repro this on main. Let's at least fix it for 3.x.


-- 
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.

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

[GitHub] [couchdb] nickva edited a comment on pull request #3363: Remove stack traces from HTTP responses

Posted by GitBox <gi...@apache.org>.
nickva edited a comment on pull request #3363:
URL: https://github.com/apache/couchdb/pull/3363#issuecomment-776124292


   @janosgyerman
   
   Very nice. It's good to have friendly error message and send stack traces to the logs. 
   
   I wonder if on `main` the specific `EXIT` clause error won't even apply. We'd have to call a `gen_server` from the request process and then the callback inside would throw an `error` and I couldn't remember if there are any calls to gen_servers made from the request process.
   
   Also, what do the logs show with and without your fix on 3.x, would you mind sharing those? And then perhaps move the PR to the 3.x branch.
   
   On main we'd have to see if these types of stacktraces can be generated at all. Maybe we'd need to add other clauses here not just an 'EXIT' one.  As a fun experiment to help generate errors you could insert various `error(some_message)` or `exit(exit_message)` expressions in lower level functions which read or write to FDB (something in fabric2_db, fabric2_fdb, erlfdb, maybe). To make it fancy you could make them a bit random and say something like:  `case random:uniform() < 0.1 of true -> exit(boom); false -> ok end.` so only 10% of the time you'd get the error.
   
   


----------------------------------------------------------------
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.

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

[GitHub] [couchdb] iilyak closed pull request #3363: Remove stack traces from HTTP responses

Posted by GitBox <gi...@apache.org>.
iilyak closed pull request #3363:
URL: https://github.com/apache/couchdb/pull/3363


   


-- 
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 pull request #3363: Remove stack traces from HTTP responses

Posted by GitBox <gi...@apache.org>.
nickva commented on pull request #3363:
URL: https://github.com/apache/couchdb/pull/3363#issuecomment-798971369


   We should still address https://github.com/apache/couchdb/pull/3363#issuecomment-776124292 I think. 
   
   Do we have an example of this clause triggering on `main`?


----------------------------------------------------------------
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.

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