peerstore: GC Purgestore races

[jsign]

Here some cleaner notes about a chat with @raulk about GC related stuff of addr peerstore.

Situation

Considering the purgeStore() implementation of the GC, I found curious that no stop-the-world was happening in the underlying book. First thought, wouldn't be racy?

Since the code isn't that large, here it is to make some references later:

func (gc *dsAddrBookGc) purgeStore() {
	select {
	case gc.running <- struct{}{}:
		defer func() { <-gc.running }()
	default:
		// yield if lookahead is running.
		return
	}

	record := &addrsRecord{AddrBookRecord: &pb.AddrBookRecord{}} // empty record to reuse and avoid allocs.
	batch, err := newCyclicBatch(gc.ab.ds, defaultOpsPerCyclicBatch)
	if err != nil {
		log.Warningf("failed while creating batch to purge GC entries: %v", err)
	}

	results, err := gc.ab.ds.Query(purgeStoreQuery)
	if err != nil {
		log.Warningf("failed while opening iterator: %v", err)
		return
	}
	defer results.Close()

	// keys: 	/peers/addrs/<peer ID b32>
	for result := range results.Next() {
		record.Reset()
		if err = record.Unmarshal(result.Value); err != nil {
			// TODO log
			continue
		}

		id := record.Id.ID
		if !record.clean() {
			continue
		}

		if err := record.flush(batch); err != nil {
			log.Warningf("failed to flush entry modified by GC for peer: &v, err: %v", id, err)
		}
		gc.ab.cache.Remove(id)
	}

	if err = batch.Commit(); err != nil {
		log.Warningf("failed to commit GC purge batch: %v", err)
	}
}

My notes:

• results is a query to fetch all the records to check if they're dirty.
• batch is a ds.Batch from ds.Batching which I don't see has any linkage with the query. Saying it differently, no information can be inferred when flushing the batch to avoid overriding new data.
• Cache eviction (gc.ab.cache.Remove(id)) is done prior to flushing the batch, so even if we don't mind overriding data, it may be possible that the cache won't represent the real state of the datastore. The cache can have newer entries that were overridden by the batch. (Maybe not bad, but weird).
• Other possible scenarios for the cache are safe: i) If the batch commit fails, nothing that bad happens (only cache was wiped out). ii) the cache implementation (ARC) is thread-safe.

The naive solution would be to lock the addrbook when the GC is running, but maybe that was a conscious decision not to be made choosing performance over consistency. If that's the case, maybe useful to drop a comment line to keep that clear.

@raulk already had some thoughts about all this but recommended to create the issue to think further actions that might be worth taking.