View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0005092 | Taler | merchant backend | public | 2017-06-23 09:29 | 2017-10-18 13:42 |
| Reporter | Marcello Stanisci | Assigned To | Christian Grothoff | Resolution deadline | |
| Implementer | Reviewer | ||||
| Release signed by | Deployed by | ||||
| Verifier | Communicator | ||||
| Data protection authority notification required | No | ||||
| CSIRT notification required | No | ||||
| Estimated effort (hours) | Actual effort (hours) | 0 | |||
| Fast-track | No | ||||
| Work package | none | ||||
| Priority | normal | Severity | minor | Reproducibility | have not tried |
| Status | closed | Resolution | fixed | ||
| Product Version | git (master) | ||||
| Target Version | 0.4 | Fixed in Version | 0.4 | ||
| Summary | 0005092: Replace GNUNET_assert with proper error handling | ||||
| Description | There is a fair number of GNUNET_assert() throughout the merchant code, and this should be avoided (where possible) as they just make it crash. Each GNUNET_assert should instead be replaced by a proper error management that eventually returns JSONs to the client. | ||||
| Tags | No tags attached. | ||||
| Well, I am not sure about "every single one". I.e. out-of-memory errors from libjansson are probably Ok to GNUNET_assert()-on. Basically, IMO asserts are OK if they indicate errors in the program logic or external components that should really never happen and where testing would be excessively difficult. But we should go over each assert and manually check that it is indeed OK. | |
| Agreed. It's worth noting that asserting makes impossible to kill forked processes, like merchant and exchange. So whenever asserts fail, merchant and exchange are to be manually killed. Is there any way to automate this murder? | |
| Well, I ended up in using more asserts than before. Basically, I fail() when the error comes from the backend so it breaks the Taler protocol; for example, if the http status is not expected or the /history response is not as expected. OTOH, I assert on "smaller" issues, like checking return values... But do take a look! | |
| Eh, maybe I'm not understanding this, but if fail() terminates the frontend, then this is very wrong. If fail() merely logs an error and gives some 500-error to the client, that's OK. | |
| This one here probably shouldn't be an assert (recently bit us): --- a/src/backend/taler-merchant-httpd_history.c +++ b/src/backend/taler-merchant-httpd_history.c @@ -45,19 +45,23 @@ pd_cb (void *cls, json_t *amount; json_t *timestamp; json_t *instance; - json_t *summary; + json_t *summary=NULL; GNUNET_log (GNUNET_ERROR_TYPE_DEBUG, "/history's row_id: %llu\n", (unsigned long long) row_id); GNUNET_assert (-1 != json_unpack ((json_t *) contract_terms, - "{s:o, s:o, s:{s:o}, s:o}", + "{s:o, s:o, s:{s:o}, s?:o}", "amount", &amount, "timestamp", ×tamp, "merchant", "instance", &instance, "summary", &summary)); | |
| I just checked, all the asserts in taler-merchant-httpd_pay.c are OK. | |
| Revised the assert in the taler-merchant-httpd_history.c, it is gone now. | |
| Assert in taler-merchant-httpd_auditors.c is OK. | |
| Asserts in taler-merchant-httpd.c are also OK. | |
| Asserts in taler-merchant-httpd_exchanges.c are OK. | |
| Asserts in taler-merchant-httpd.c are OK. | |
| Fixed assert in taler-merchant-httpd_proposal.c, rest is OK. | |
| Fixed another easy one in responses. | |
| Asserts in taler-merchant-httpd_track-transaction.c are OK. | |
| Asserts in taler-merchant-httpd_track-transfer.c are OK. | |
| Declaring victory as of cd3071d..4bea239 | |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2017-06-23 09:29 | Marcello Stanisci | New Issue | |
| 2017-06-23 09:29 | Marcello Stanisci | Status | new => assigned |
| 2017-06-23 09:29 | Marcello Stanisci | Assigned To | => Marcello Stanisci |
| 2017-06-23 09:33 | Marcello Stanisci | Target Version | => 0.5 |
| 2017-06-29 18:08 | Christian Grothoff | Note Added: 0012284 | |
| 2017-07-11 14:47 | Marcello Stanisci | Note Added: 0012326 | |
| 2017-07-11 14:48 | Marcello Stanisci | Note Edited: 0012326 | |
| 2017-07-17 08:34 | Marcello Stanisci | Note Added: 0012340 | |
| 2017-07-17 08:34 | Marcello Stanisci | Assigned To | Marcello Stanisci => Christian Grothoff |
| 2017-07-20 07:20 | Christian Grothoff | Note Added: 0012354 | |
| 2017-08-26 06:59 | Christian Grothoff | Note Added: 0012392 | |
| 2017-09-09 19:59 | Christian Grothoff | Note Added: 0012411 | |
| 2017-09-09 20:10 | Christian Grothoff | Note Added: 0012412 | |
| 2017-09-09 20:14 | Christian Grothoff | Note Added: 0012413 | |
| 2017-09-09 20:14 | Christian Grothoff | Note Added: 0012414 | |
| 2017-09-09 20:19 | Christian Grothoff | Note Added: 0012415 | |
| 2017-09-09 20:20 | Christian Grothoff | Note Added: 0012416 | |
| 2017-09-09 20:27 | Christian Grothoff | Note Added: 0012417 | |
| 2017-09-09 20:28 | Christian Grothoff | Note Added: 0012418 | |
| 2017-09-09 20:33 | Christian Grothoff | Note Added: 0012419 | |
| 2017-09-09 20:34 | Christian Grothoff | Note Added: 0012420 | |
| 2017-09-09 20:42 | Christian Grothoff | Status | assigned => informing |
| 2017-09-09 20:42 | Christian Grothoff | Resolution | open => fixed |
| 2017-09-09 20:42 | Christian Grothoff | Fixed in Version | => 0.4 |
| 2017-09-09 20:42 | Christian Grothoff | Note Added: 0012421 | |
| 2017-09-09 20:42 | Christian Grothoff | Product Version | => git (master) |
| 2017-09-09 20:42 | Christian Grothoff | Target Version | 0.5 => 0.4 |
| 2017-10-18 13:42 | Christian Grothoff | Status | informing => closed |
| 2025-03-22 13:09 | Christian Grothoff | Category | mechant backend => merchant backend |