View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0005092Talermechant backendpublic2017-06-23 11:292017-10-18 15:42
ReporterMarcello Stanisci Assigned ToChristian Grothoff  
PrioritynormalSeverityminorReproducibilityhave not tried
Status closedResolutionfixed 
Product Versiongit (master) 
Target Version0.4Fixed in Version0.4 
Summary0005092: Replace GNUNET_assert with proper error handling
DescriptionThere 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.
TagsNo tags attached.

Activities

Christian Grothoff

2017-06-29 20:08

manager   ~0012284

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.

Marcello Stanisci

2017-07-11 16:47

reporter   ~0012326

Last edited: 2017-07-11 16:48

 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?

Marcello Stanisci

2017-07-17 10:34

reporter   ~0012340

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!

Christian Grothoff

2017-07-20 09:20

manager   ~0012354

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.

Christian Grothoff

2017-08-26 08:59

manager   ~0012392

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", &timestamp,
                                     "merchant", "instance", &instance,
                                     "summary", &summary));

Christian Grothoff

2017-09-09 21:59

manager   ~0012411

I just checked, all the asserts in taler-merchant-httpd_pay.c are OK.

Christian Grothoff

2017-09-09 22:10

manager   ~0012412

Revised the assert in the taler-merchant-httpd_history.c, it is gone now.

Christian Grothoff

2017-09-09 22:14

manager   ~0012413

Assert in taler-merchant-httpd_auditors.c is OK.

Christian Grothoff

2017-09-09 22:14

manager   ~0012414

Asserts in taler-merchant-httpd.c are also OK.

Christian Grothoff

2017-09-09 22:19

manager   ~0012415

Asserts in taler-merchant-httpd_exchanges.c are OK.

Christian Grothoff

2017-09-09 22:20

manager   ~0012416

Asserts in taler-merchant-httpd.c are OK.

Christian Grothoff

2017-09-09 22:27

manager   ~0012417

Fixed assert in taler-merchant-httpd_proposal.c, rest is OK.

Christian Grothoff

2017-09-09 22:28

manager   ~0012418

Fixed another easy one in responses.

Christian Grothoff

2017-09-09 22:33

manager   ~0012419

Asserts in taler-merchant-httpd_track-transaction.c are OK.

Christian Grothoff

2017-09-09 22:34

manager   ~0012420

Asserts in taler-merchant-httpd_track-transfer.c are OK.

Christian Grothoff

2017-09-09 22:42

manager   ~0012421

Declaring victory as of cd3071d..4bea239

Issue History

Date Modified Username Field Change
2017-06-23 11:29 Marcello Stanisci New Issue
2017-06-23 11:29 Marcello Stanisci Status new => assigned
2017-06-23 11:29 Marcello Stanisci Assigned To => Marcello Stanisci
2017-06-23 11:33 Marcello Stanisci Target Version => 0.5
2017-06-29 20:08 Christian Grothoff Note Added: 0012284
2017-07-11 16:47 Marcello Stanisci Note Added: 0012326
2017-07-11 16:48 Marcello Stanisci Note Edited: 0012326
2017-07-17 10:34 Marcello Stanisci Note Added: 0012340
2017-07-17 10:34 Marcello Stanisci Assigned To Marcello Stanisci => Christian Grothoff
2017-07-20 09:20 Christian Grothoff Note Added: 0012354
2017-08-26 08:59 Christian Grothoff Note Added: 0012392
2017-09-09 21:59 Christian Grothoff Note Added: 0012411
2017-09-09 22:10 Christian Grothoff Note Added: 0012412
2017-09-09 22:14 Christian Grothoff Note Added: 0012413
2017-09-09 22:14 Christian Grothoff Note Added: 0012414
2017-09-09 22:19 Christian Grothoff Note Added: 0012415
2017-09-09 22:20 Christian Grothoff Note Added: 0012416
2017-09-09 22:27 Christian Grothoff Note Added: 0012417
2017-09-09 22:28 Christian Grothoff Note Added: 0012418
2017-09-09 22:33 Christian Grothoff Note Added: 0012419
2017-09-09 22:34 Christian Grothoff Note Added: 0012420
2017-09-09 22:42 Christian Grothoff Status assigned => resolved
2017-09-09 22:42 Christian Grothoff Resolution open => fixed
2017-09-09 22:42 Christian Grothoff Fixed in Version => 0.4
2017-09-09 22:42 Christian Grothoff Note Added: 0012421
2017-09-09 22:42 Christian Grothoff Product Version => git (master)
2017-09-09 22:42 Christian Grothoff Target Version 0.5 => 0.4
2017-10-18 15:42 Christian Grothoff Status resolved => closed