Merge branch 'jono/fileTransferLoadFix' into 'release'

File transfer KV fixes

See merge request !189

bindings/fileTransfer.go

@@ -163,12 +163,13 @@ func (f *FileTransfer) RegisterSendProgressCallback(transferID []byte,
 
 // Resend resends a file if sending fails. This function should only be called
 // if the interfaces.SentProgressCallback returns an error.
-func (f *FileTransfer) Resend(transferID []byte) error {
+// Resend is not currently implemented.
+/*func (f *FileTransfer) Resend(transferID []byte) error {
 	// Unmarshal transfer ID
 	tid := ftCrypto.UnmarshalTransferID(transferID)
 
 	return f.m.Resend(tid)
-}
+}*/
 
 // CloseSend deletes a sent file transfer from the sent transfer map and from
 // storage once a transfer has completed or reached the retry limit. Returns an

storage/fileTransfer/receiveFileTransfers.go

@@ -29,7 +29,7 @@ const (
 const (
 	saveReceivedTransfersListErr = "failed to save list of received items in transfer map to storage: %+v"
 	loadReceivedTransfersListErr = "failed to load list of received items in transfer map from storage: %+v"
-	loadReceivedFileTransfersErr = "failed to load received transfers from storage: %+v"
+	loadReceivedFileTransfersErr = "[FT] Failed to load received transfers from storage: %+v"
 
 	newReceivedTransferErr    = "failed to create new received transfer: %+v"
 	getReceivedTransferErr    = "received transfer with ID %s not found"
@@ -37,6 +37,10 @@ const (
 	noFingerprintErr          = "no part found with fingerprint %s"
 	addPartErr                = "failed to add part to transfer %s: %+v"
 	deleteReceivedTransferErr = "failed to delete received transfer with ID %s from store: %+v"
+
+	// ReceivedFileTransfersStore.load
+	loadReceivedTransferWarn    = "[FT] Failed to load received file transfer %d of %d with ID %s: %v"
+	loadReceivedTransfersAllErr = "failed to load all %d transfers"
 )
 
 // ReceivedFileTransfersStore contains information for tracking a received
@@ -265,7 +269,8 @@ func NewOrLoadReceivedFileTransfersStore(kv *versioned.KV) (
 	// Load transfers and fingerprints into the maps
 	err = rft.load(transfersList)
 	if err != nil {
-		return nil, errors.Errorf(loadReceivedFileTransfersErr, err)
+		jww.ERROR.Printf(loadReceivedFileTransfersErr, err)
+		return NewReceivedFileTransfersStore(kv)
 	}
 
 	return rft, nil
@@ -304,12 +309,16 @@ func (rft *ReceivedFileTransfersStore) loadTransfersList() (
 // map. Also adds all unused fingerprints in each ReceivedTransfer to the info
 // map.
 func (rft *ReceivedFileTransfersStore) load(list []ftCrypto.TransferID) error {
+	var errCount int
+
 	// Load each sentTransfer from storage into the map
-	for _, tid := range list {
+	for i, tid := range list {
 		// Load the transfer with the given transfer ID from storage
 		rt, err := loadReceivedTransfer(tid, rft.kv)
 		if err != nil {
-			return err
+			jww.WARN.Printf(loadReceivedTransferWarn, i, len(list), tid, err)
+			errCount++
+			continue
 		}
 
 		// Add transfer to transfer map
@@ -329,6 +338,11 @@ func (rft *ReceivedFileTransfersStore) load(list []ftCrypto.TransferID) error {
 		}
 	}
 
+	// Return an error if all transfers failed to load
+	if errCount == len(list) {
+		return errors.Errorf(loadReceivedTransfersAllErr, len(list))
+	}
+
 	return nil
 }
 

storage/fileTransfer/receiveFileTransfers_test.go

@@ -654,30 +654,6 @@ func TestNewOrLoadReceivedFileTransfersStore_NewReceivedFileTransfersStore(t *te
 	}
 }
 
-// Error path: tests that NewOrLoadReceivedFileTransfersStore returns the
-// expected error when the first transfer loaded from storage does not exist.
-func TestNewOrLoadReceivedFileTransfersStore_NoTransferInStorageError(t *testing.T) {
-	kv := versioned.NewKV(make(ekv.Memstore))
-	expectedErr := strings.Split(loadReceivedFileTransfersErr, "%")[0]
-
-	// Save list of one transfer ID to storage
-	obj := &versioned.Object{
-		Version:   receivedFileTransfersStoreVersion,
-		Timestamp: netTime.Now(),
-		Data:      ftCrypto.UnmarshalTransferID([]byte("testID_01")).Bytes(),
-	}
-	err := kv.Prefix(receivedFileTransfersStorePrefix).Set(
-		receivedFileTransfersStoreKey, receivedFileTransfersStoreVersion, obj)
-
-	// Load ReceivedFileTransfersStore from storage
-	_, err = NewOrLoadReceivedFileTransfersStore(kv)
-	if err == nil || !strings.Contains(err.Error(), expectedErr) {
-		t.Errorf("NewOrLoadReceivedFileTransfersStore did not return the "+
-			"expected error when there is no transfer saved in storage."+
-			"\nexpected: %s\nreceived: %+v", expectedErr, err)
-	}
-}
-
 // Tests that the list saved by ReceivedFileTransfersStore.saveTransfersList
 // matches the list loaded by ReceivedFileTransfersStore.load.
 func TestReceivedFileTransfersStore_saveTransfersList_loadTransfersList(t *testing.T) {
@@ -802,6 +778,57 @@ func TestReceivedFileTransfersStore_load(t *testing.T) {
 	}
 }
 
+// Error path: tests that ReceivedFileTransfersStore.load returns an error when
+// all file transfers fail to load from storage.
+func TestReceivedFileTransfersStore_load_AllFail(t *testing.T) {
+	kv := versioned.NewKV(make(ekv.Memstore))
+	rft, err := NewReceivedFileTransfersStore(kv)
+	if err != nil {
+		t.Fatalf("Failed to create new ReceivedFileTransfersStore: %+v", err)
+	}
+
+	// Fill map with transfers
+	idList := make([]ftCrypto.TransferID, 7)
+	for i := range idList {
+		prng := NewPrng(int64(i))
+		key, _ := ftCrypto.NewTransferKey(prng)
+		mac := []byte("transferMAC")
+
+		idList[i], err = rft.AddTransfer(key, mac, 256, 16, 24, prng)
+		if err != nil {
+			t.Errorf("Failed to add new transfer #%d: %+v", i, err)
+		}
+
+		err = rft.DeleteTransfer(idList[i])
+		if err != nil {
+			t.Errorf("Failed to delete transfer: %+v", err)
+		}
+	}
+
+	// Save the list
+	err = rft.saveTransfersList()
+	if err != nil {
+		t.Errorf("saveTransfersList returned an error: %+v", err)
+	}
+
+	// Build new ReceivedFileTransfersStore
+	newRFT := &ReceivedFileTransfersStore{
+		transfers: make(map[ftCrypto.TransferID]*ReceivedTransfer),
+		info:      make(map[format.Fingerprint]*partInfo),
+		kv:        kv.Prefix(receivedFileTransfersStorePrefix),
+	}
+
+	expectedErr := fmt.Sprintf(loadReceivedTransfersAllErr, len(idList))
+
+	// Load saved transfers from storage
+	err = newRFT.load(idList)
+	if err == nil || err.Error() != expectedErr {
+		t.Errorf("load did not return the expected error when none of the "+
+			"transfer could be loaded from storage."+
+			"\nexpected: %s\nreceived: %+v", expectedErr, err)
+	}
+}
+
 // Tests that a transfer list marshalled with
 // ReceivedFileTransfersStore.marshalTransfersList and unmarshalled with
 // unmarshalTransfersList matches the original.

storage/fileTransfer/sentFileTransfers.go

@@ -32,12 +32,16 @@ const (
 const (
 	saveSentTransfersListErr = "failed to save list of sent items in transfer map to storage: %+v"
 	loadSentTransfersListErr = "failed to load list of sent items in transfer map from storage: %+v"
-	loadSentTransfersErr     = "failed to load sent transfers from storage: %+v"
+	loadSentTransfersErr     = "[FT] Failed to load sent transfers from storage: %+v"
 
 	newSentTransferErr    = "failed to create new sent transfer: %+v"
 	getSentTransferErr    = "sent file transfer not found"
 	cancelCallbackErr     = "[FT] Transfer with ID %s: %+v"
 	deleteSentTransferErr = "failed to delete sent transfer with ID %s from store: %+v"
+
+	// SentFileTransfersStore.loadTransfers
+	loadSentTransferWarn    = "[FT] Failed to load sent file transfer %d of %d with ID %s: %v"
+	loadSentTransfersAllErr = "failed to load all %d transfers"
 )
 
 // SentFileTransfersStore contains information for tracking sent file transfers.
@@ -254,8 +258,7 @@ func NewOrLoadSentFileTransfersStore(kv *versioned.KV) (*SentFileTransfersStore,
 	vo, err := sft.kv.Get(
 		sentFileTransfersStoreKey, sentFileTransfersStoreVersion)
 	if err != nil {
-		newSFT, err := NewSentFileTransfersStore(kv)
-		return newSFT, err
+		return NewSentFileTransfersStore(kv)
 	}
 
 	// Unmarshal data into list of saved transfer IDs
@@ -264,7 +267,8 @@ func NewOrLoadSentFileTransfersStore(kv *versioned.KV) (*SentFileTransfersStore,
 	// Load each transfer in the list from storage into the map
 	err = sft.loadTransfers(transfersList)
 	if err != nil {
-		return nil, errors.Errorf(loadSentTransfersErr, err)
+		jww.WARN.Printf(loadSentTransfersErr, err)
+		return NewSentFileTransfersStore(kv)
 	}
 
 	return sft, nil
@@ -304,15 +308,22 @@ func (sft *SentFileTransfersStore) loadTransfersList() ([]ftCrypto.TransferID,
 // to add them back into the queue.
 func (sft *SentFileTransfersStore) loadTransfers(list []ftCrypto.TransferID) error {
 	var err error
+	var errCount int
 
 	// Load each sentTransfer from storage into the map
-	for _, tid := range list {
+	for i, tid := range list {
 		sft.transfers[tid], err = loadSentTransfer(tid, sft.kv)
 		if err != nil {
-			return err
+			jww.WARN.Printf(loadSentTransferWarn, i, len(list), tid, err)
+			errCount++
 		}
 	}
 
+	// Return an error if all transfers failed to load
+	if errCount == len(list) {
+		return errors.Errorf(loadSentTransfersAllErr, len(list))
+	}
+
 	return nil
 }
 

storage/fileTransfer/sentFileTransfers_test.go

@@ -8,6 +8,7 @@
 package fileTransfer
 
 import (
+	"fmt"
 	"gitlab.com/elixxir/client/storage/versioned"
 	ftCrypto "gitlab.com/elixxir/crypto/fileTransfer"
 	"gitlab.com/elixxir/ekv"
@@ -582,30 +583,6 @@ func TestNewOrLoadSentFileTransfersStore_NewSentFileTransfersStore(t *testing.T)
 	}
 }
 
-// Error path: tests that the NewOrLoadSentFileTransfersStore returns the
-// expected error when the first transfer loaded from storage does not exist.
-func TestNewOrLoadSentFileTransfersStore_NoTransferInStorageError(t *testing.T) {
-	kv := versioned.NewKV(make(ekv.Memstore))
-	expectedErr := strings.Split(loadSentTransfersErr, "%")[0]
-
-	// Save list of one transfer ID to storage
-	obj := &versioned.Object{
-		Version:   sentFileTransfersStoreVersion,
-		Timestamp: netTime.Now(),
-		Data:      ftCrypto.UnmarshalTransferID([]byte("testID_01")).Bytes(),
-	}
-	err := kv.Prefix(sentFileTransfersStorePrefix).Set(
-		sentFileTransfersStoreKey, sentFileTransfersStoreVersion, obj)
-
-	// Load SentFileTransfersStore from storage
-	_, err = NewOrLoadSentFileTransfersStore(kv)
-	if err == nil || !strings.Contains(err.Error(), expectedErr) {
-		t.Errorf("NewOrLoadSentFileTransfersStore did not return the expected "+
-			"error when there is no transfer saved in storage."+
-			"\nexpected: %s\nreceived: %+v", expectedErr, err)
-	}
-}
-
 // Tests that SentFileTransfersStore.saveTransfersList saves all the transfer
 // IDs to storage by loading them from storage via
 // SentFileTransfersStore.loadTransfersList and comparing the list to the list
@@ -646,7 +623,7 @@ func TestSentFileTransfersStore_saveTransfersList_loadTransfersList(t *testing.T
 }
 
 // Tests that the transfer loaded by SentFileTransfersStore.loadTransfers from
-// storage matches the original in memory
+// storage matches the original in memory.
 func TestSentFileTransfersStore_loadTransfers(t *testing.T) {
 	kv := versioned.NewKV(make(ekv.Memstore))
 	sft := &SentFileTransfersStore{
@@ -686,6 +663,37 @@ func TestSentFileTransfersStore_loadTransfers(t *testing.T) {
 	}
 }
 
+// Error path: tests that SentFileTransfersStore.loadTransfers returns an error
+// when all file transfers fail to load from storage.
+func TestSentFileTransfersStore_loadTransfers_AllErrors(t *testing.T) {
+	kv := versioned.NewKV(make(ekv.Memstore)).Prefix(sentFileTransfersStorePrefix)
+
+	// Add 10 transfers to map in memory
+	list := make([]ftCrypto.TransferID, 10)
+	for i := range list {
+		tid, st := newRandomSentTransfer(16, 24, kv, t)
+		list[i] = tid
+		err := st.delete()
+		if err != nil {
+			t.Errorf("Failed to delete transfer: %+v", err)
+		}
+	}
+
+	expectedErr := fmt.Sprintf(loadSentTransfersAllErr, len(list))
+
+	// Load the transfers into a new SentFileTransfersStore
+	loadedSft := &SentFileTransfersStore{
+		transfers: make(map[ftCrypto.TransferID]*SentTransfer),
+		kv:        kv,
+	}
+	err := loadedSft.loadTransfers(list)
+	if err == nil || err.Error() != expectedErr {
+		t.Errorf("loadTransfers did not return the expected error when none "+
+			"of the transfer could be loaded from storage."+
+			"\nexpected: %s\nreceived: %+v", expectedErr, err)
+	}
+}
+
 // Tests that a transfer list marshalled with
 // SentFileTransfersStore.marshalTransfersList and unmarshalled with
 // unmarshalTransfersList matches the original.