View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0004532 | GNUnet | peerstore | public | 2016-05-25 11:58 | 2018-06-07 00:24 |
| Reporter | schanzen | Assigned To | Christian Grothoff | ||
| Priority | normal | Severity | minor | Reproducibility | always |
| Status | closed | Resolution | fixed | ||
| Product Version | Git master | ||||
| Target Version | 0.11.0pre66 | Fixed in Version | 0.11.0pre66 | ||
| Summary | 0004532: Peerstore API inconsistent | ||||
| Description | Peerstore API for iteration is buggy: 130 /** 131 * Function called by PEERSTORE for each matching record. 132 * 133 * @param cls closure 134 * @param record peerstore record information 135 * @param emsg error message, or NULL if no errors 136 * @return #GNUNET_YES to continue iterating, #GNUNET_NO to stop 137 */ 138 typedef int 139 (*GNUNET_PEERSTORE_Processor) (void *cls, 140 const struct GNUNET_PEERSTORE_Record *record, 141 const char *emsg); The processor is supposed to return GNUNET_YES if the iteration should continue. However, if you look into the plugin peerstore/plugin_peerstore_sqlite.c you can see that the return value of the processor is ignored and iteration continues. I don't know if fixing this behaviour will break things so maybe it is sensible to change the signature of the processor?? | ||||
| Tags | No tags attached. | ||||
| related to | 0004541 | closed | Christian Grothoff | gnunet-service-fs crash |
|
|
Why do you say the API is broken, if it would be equally fair to say that the sqlite implementation is broken? It seems to me the simplest and least risky fix is to change the sqlite code to check the return value and stop the iteration if it is not GNUNET_YES. If that unexpectedly breaks any client, it's clear that the client violated the API and that's then again easily fixed with that client. The API itself seems fine to me. |
|
|
Hmm well I say inconsistent because I think it is a mix between 2 approaches: Approach 1: See MultiHashMap iterator: It simply iterates over all entries and you get to decide weather or not you want to continue (current documented behavior, but not implemented as such) Approach 2: See Namestore: You have to call iterate for every result. NULL means end of results. Current API: Mix between 1 and 2. You don't get to choose to stop and you get NULL at the end. The problem: I assume that components that use this API expect it to behave exactly like this (return NULL when finished). But how can this be fixed if we allow a GNUNET_NO to end the iteration?? Should we still return a NULL result for compatibility? Or fix all implementations? I think we should decide on a consistent approach (either 1 or 2). If we go for 2 it means potentially change a lot of components that use the API. If the go for 1 maybe nothing breaks if we are lucky. (We have to remove the NULL check though). Oh of course there is approach 3: Leave it with a NULL terminator and allow to abort but be _inconsistent_ with other APIs. |
|
|
Ok, then clearly making this decision requires looking at all of the uses of the code. Fun. Won't happen overnight ;-). |
|
|
Looking through the code there is another "problem". The processor is used by components implementing this API AND the peerstore service calling the plugin. I.e. it is also a gnunet service call. Shouldn't it be like namestore, then? What I do not understand is if it is actually "safe" to pump out all result messages synchronously by the service (gnunet-service-peerstore.c 386ff) AND allow the caller to cancel the iteration. Either the API is async and we need: iterator_start () iterator_next () iterator_cancel () or it is not and we only need iterate () But it is service so it IS async, right?? I think this API needs a major revision. Considering it is only used by a single service (fs) it should not break a lot (?) We should also use a different processor for the plugin callback that is used by the service. This processor does not even need to be public. BUT: If we change the Peerstore iterate API to be async like namestore, we might have to change the SQL calls in the plugin as well. Might not be backwards compatible with older DBs? |
|
|
If the service pumps out all of the results and the client 'cancels', what we simply do, is inside the API, we throw away all of the results after the cancellation. That is "safe", and often more efficient than sending a 'next' back to the service after each result (depending, of course, on how many results we get after cancel and how expensive those are to produce, but in this case I think it should be OK). |
|
|
Okay. Then I suggest we simply change the signature of the processor. Cancelling the iteration in two different ways seems overkill (And one of the is buggy right now anyway). |
|
|
Fine with me. |
|
|
Fixed in r37211. |
|
|
Eh, please remove the printf()'s that SVN 37211 introduced... |
|
|
Oops -> r37237. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2016-05-25 11:58 | schanzen | New Issue | |
| 2016-05-25 11:58 | schanzen | Status | new => assigned |
| 2016-05-25 11:58 | schanzen | Assigned To | => Christian Grothoff |
| 2016-05-25 14:47 | schanzen | Summary | Peerstire API inconsistent => Peerstore API inconsistent |
| 2016-05-25 21:08 | Christian Grothoff | Note Added: 0010749 | |
| 2016-05-25 21:08 | Christian Grothoff | Target Version | Git master => 0.11.0pre66 |
| 2016-05-25 21:17 | Christian Grothoff | Status | assigned => feedback |
| 2016-05-25 21:50 | schanzen | Note Added: 0010759 | |
| 2016-05-25 21:50 | schanzen | Status | feedback => assigned |
| 2016-05-25 21:53 | Christian Grothoff | Note Added: 0010760 | |
| 2016-05-26 13:55 | schanzen | Note Added: 0010761 | |
| 2016-05-26 14:07 | Christian Grothoff | Note Added: 0010762 | |
| 2016-05-26 14:20 | schanzen | Note Added: 0010764 | |
| 2016-05-26 15:33 | Christian Grothoff | Note Added: 0010768 | |
| 2016-05-29 13:20 | schanzen | Note Added: 0010776 | |
| 2016-05-29 13:20 | schanzen | Status | assigned => resolved |
| 2016-05-29 13:20 | schanzen | Fixed in Version | => Git master |
| 2016-05-29 13:20 | schanzen | Resolution | open => fixed |
| 2016-05-29 19:44 | Christian Grothoff | Note Added: 0010777 | |
| 2016-05-29 19:44 | Christian Grothoff | Status | resolved => assigned |
| 2016-06-02 19:22 | Christian Grothoff | Relationship added | related to 0004541 |
| 2016-06-02 19:35 | schanzen | Note Added: 0010852 | |
| 2016-06-02 19:35 | schanzen | Status | assigned => resolved |
| 2016-06-03 09:20 | Christian Grothoff | Fixed in Version | Git master => 0.11.0pre66 |
| 2018-06-07 00:24 | Christian Grothoff | Status | resolved => closed |
