Skip to content
This repository has been archived by the owner on Aug 11, 2021. It is now read-only.

fix: race condition in getFormat #299

Merged
merged 2 commits into from
Jun 15, 2021
Merged

fix: race condition in getFormat #299

merged 2 commits into from
Jun 15, 2021

Conversation

matheus23
Copy link
Contributor

@matheus23 matheus23 commented May 14, 2021

There can be a race condition in ipld, if multiple requests to getFormat happen concurrently. While one of the requests awaits this.loadFormat, another request might finish loading it, and both end up calling this.addFormat.

But if multiple end up calling that function then this.addFormat throws an error:

js-ipld/src/index.js

Lines 65 to 76 in 021b419

addFormat (format) {
const codec = format.codec
if (this.resolvers[format.codec]) {
const codecName = multicodec.getNameFromCode(codec)
throw new Error(`Resolver already exists for codec "${codecName}"`)
}
this.resolvers[codec] = format
return this
}

The solution is a basic software transactional memory (STM) idea: If your reads are invalidated during your transaction, retry the transaction.

In its purest form an STM solution would mean:

async getFormat (codec) {
  // TODO vmx 2019-01-24: Once all CIDs support accessing the codec code
  // instead of the name, remove this part
  if (typeof codec === 'string') {
    codec = multicodec.getCodeFromName(codec)
  }

  if (this.resolvers[codec]) {
    return this.resolvers[codec]
  }

  // If not supported, attempt to dynamically load this format
  const format = await this.loadFormat(codec)
  if (!this.resolvers[codec]) { // detect read invalidation
    return getFormat(codec) // retry
  }
  return format
}

But we can manually inline the getFormat call and slightly improve the if-condition to be more readable and we end up with the code in this PR.

@lidel
Copy link

lidel commented May 31, 2021

@vmx thoughts? (how does this work in the new ipld stuff?)

Copy link
Member

@vmx vmx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good to me.

The new js-multiformats stuff doesn't have any similar code. Likely you would implement something like this directly in js-ipfs.

@lidel
Copy link

lidel commented Jun 7, 2021

@vmx mind merging & releasing? (I lack permissions / know-how :^))

@achingbrain achingbrain changed the title Fix a race condition in getFormat fix: race condition in getFormat Jun 15, 2021
@achingbrain achingbrain merged commit a53ebcf into ipld:master Jun 15, 2021
@matheus23 matheus23 deleted the patch-1 branch June 15, 2021 19:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants