Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skylink race #153

Merged
merged 5 commits into from
Mar 17, 2022
Merged

Skylink race #153

merged 5 commits into from
Mar 17, 2022

Conversation

ro-tex
Copy link
Contributor

@ro-tex ro-tex commented Mar 16, 2022

PULL REQUEST

Overview

This PR fixes a problem caused by a race condition.

The race condition happens when we try to fetch a skylink from the database, we don't find it, so we decide to insert it. When inserting it, we use upsert in order to avoid overwriting the ID if it just so happens that another server creates that exact skylink in that very moment. The issue is that in that case the upsert operation does not do anything and it does not return an UpsertID. This leaves the skylink record returned by the function with a valid hash but an empty ID. This empty ID later on causes this skylink to be evaluated as invalid.

This PR makes two changes:

  • instead of "invalid skylink" we now return "skylink doesn't exist" to better capture the situation of an empty ID
  • we specifically check whether the UpsertID we get is empty and if that's the case we fetch the skylink record from the database
  • we replace the find and if it's not there upsert flow with a single call to findOneAndUpdate that will only perform an update if if doesn't find the record in the DB (thanks to $setOnInsert), thus giving us an atomic way to either find or insert the record.

Example for Visual Changes

Checklist

Review and complete the checklist to ensure that the PR is complete before assigned to an approver.

  • All new methods or updated methods have clear docstrings
  • Testing added or updated for new methods
  • Verify if any changes impact the WebPortal Health Checks
  • Approriate documentation updated
  • Changelog file created

Issues Closed

@ro-tex ro-tex self-assigned this Mar 16, 2022
database/skylink.go Outdated Show resolved Hide resolved
Copy link

@peterjan peterjan left a comment

Choose a reason for hiding this comment

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

👍

@peterjan peterjan merged commit 800ba57 into main Mar 17, 2022
@peterjan peterjan deleted the ivo/fix_skylink_race branch March 17, 2022 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants