View Issue Details

IDProjectCategoryView StatusLast Update
0004532GNUnetpeerstorepublic2018-06-07 00:24
ReporterschanzenAssigned ToChristian Grothoff 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product VersionSVN HEAD 
Target Version0.11.0pre66Fixed in Version0.11.0pre66 
Summary0004532: Peerstore API inconsistent
DescriptionPeerstore 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??
TagsNo tags attached.

Relationships

related to 0004541 closedChristian Grothoff gnunet-service-fs crash 

Activities

Christian Grothoff

2016-05-25 21:08

manager   ~0010749

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.

schanzen

2016-05-25 21:50

developer   ~0010759

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.

Christian Grothoff

2016-05-25 21:53

manager   ~0010760

Ok, then clearly making this decision requires looking at all of the uses of the code. Fun. Won't happen overnight ;-).

schanzen

2016-05-26 13:55

developer   ~0010761

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?

Christian Grothoff

2016-05-26 14:07

manager   ~0010762

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

schanzen

2016-05-26 14:20

developer   ~0010764

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

Christian Grothoff

2016-05-26 15:33

manager   ~0010768

Fine with me.

schanzen

2016-05-29 13:20

developer   ~0010776

Fixed in r37211.

Christian Grothoff

2016-05-29 19:44

manager   ~0010777

Eh, please remove the printf()'s that SVN 37211 introduced...

schanzen

2016-06-02 19:35

developer   ~0010852

Oops -> r37237.

Issue History

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 SVN HEAD => 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 => SVN HEAD
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 SVN HEAD => 0.11.0pre66
2018-06-07 00:24 Christian Grothoff Status resolved => closed