calling setMetadata in a series of promises results in unpredictable values being saved

[eleith]

Environment details

linux, node 8, using firebase admin which has @google-cloud/storage 1.6

Steps to reproduce

(async () => {
    const bucket = admin.storage().bucket("mybucket");
    const file = bucket.file("myfile");
    const promises = [];
  
    [2, 3, 4, 5].forEach(i => {
      promises.push(
        file.setMetadata({
          metadata: {
            [`test-${i}`]: "testing"
          }
        })
      );
    });
  
    await Promise.all(promises);
  })();

this results in unpredictable metadata being set to the file.

obvious workaround is to submit the metadata all in one call, but abstracted out far enough, it's possible to not realize this is whats happening underneath the hood. (imagine a promise chain returning the values you want to submit and then just calling down the chain of functions that lead to a setMetadata)

is it possible to document some best practices here https://cloud.google.com/nodejs/docs/reference/storage/1.7.x/File#setMetadata to raise awareness if this is expected?

[eleith]

another workaround i was able to put together is just to wrap all calls in an async function and loop through the array calling await on each promise one by one

(async () => {
    const bucket = admin.storage().bucket("mybucket");
    const file = bucket.file("myfile");
    const promises = [];
  
    [2, 3, 4, 5].forEach(i => {
      promises.push(async() => {
        await file.setMetadata({
          metadata: {
            [`test-${i}`]: "testing"
          }
        })
      });
    });

    for(const promise of promises) {
     await promise()
   }  
  })();

[stephenplusplus]

Here's what we currently have:

Merge the given metadata with the current remote file's metadata. This will set metadata if it was previously unset or update previously set metadata. To unset previously set metadata, set its value to null.

Would you be willing to contribute a PR to suggest documentation that you would find more helpful?

[eleith]

yes, i'ld be willing to contribute once i can better understand the underlying behavior (and to get confirmation there isn't a bug in my example code).

my goal is to set the metadata object

{
  "apple-1": "testing",
  "apple-2": "testing",
  "apple-3": "testing",
  "apple-4": "testing
}

by executing multiple calls to setMetadata in series, the metadata object is set as desired:

(async () => {
    const bucket = admin.storage().bucket("mybucket");
    const file = bucket.file("myfile");
    const promises = [];
  
    [2, 3, 4, 5].forEach(i => {
      promises.push(async() => {
        console.log(`starting ${i}`);
        await file.setMetadata({
          metadata: {
            [`apple-${i}`]: "testing"
          }
        });
        console.log(`ending ${i}`);
      });
    });

    for(const promise of promises) {
     await promise()
   }  
  })();

however, by executing multiple calls to setMetadata in parallel, i get different results from time to time, with various fields getting set and never the same ones on subsequent runs:

(async () => {
    const bucket = admin.storage().bucket("mybucket");
    const file = bucket.file("myfile");
    const promises = [];
  
        [8, 9, 11, 12].forEach(i => {
      promises.push(
        new Promise(resolve => {
          console.log(`starting ${i}`);
          file
            .setMetadata({
              metadata: {
                [`apple-${i}`]: "testing"
              }
            })
            .then(() => {
             console.log(`ending ${i}`);
              resolve();
            });
        })
      );
    });
  
    await Promise.all(promises);
  })();

in both parallel / series versions, we get the console output that confirms all promises are fully executed.

my current theory is that if you make multiple calls to setMetadata, you must ensure the previous call has finished in order to guarantee the results. however, if you happen to make a call before a previous one returns, you may end up in a scenario where some eventual consistency plumbing could shake out what was set in a previous call.

if the above theory is in the right direction, i could propose some documentation in a PR suggesting callers of this method be aware of this behavior and to avoid parallel calls.

[jkwlui]

my current theory is that if you make multiple calls to setMetadata, you must ensure the previous call has finished in order to guarantee the results.

closing via #504