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

Implement mutex around file mutation http handlers #247

Merged
merged 9 commits into from Jul 30, 2019

Conversation

@zone117x
Copy link
Member

commented Jun 27, 2019

Goal of PR

Implement mutex around file http handlers that perform file mutations: POST /store/address/file and DELETE /delete/address/file.

Implementation

A mutex is unique to a address/file endpoint. When an operation is in progress for a given resource, subsequence requests to the modify the resource are immediately rejected with an HTTP 409 Conflict.

Manual Testing

Instantiating many concurrent POST or DELETE requests to the same endpoint, using something like curl or a load tester.

Automated Testing

Tests implemented for full coverage of the mutex implementation and http conflict error paths.

Submission Checklist

  • Based on correct branch: feature submissions should be on develop, hotfixes should be on master

  • The code passes our eslint definitions, unit tests, and
    contains correct TypeFlow annotations.

  • Submission contains tests that cover any and all new functionality or code changes.

  • Submission documents any new features or endpoints, and describes how developers
    would be expected to interact with them.

  • Author has agreed to our contributor's agreement.

@zone117x zone117x requested review from jcnelson and kantai Jun 27, 2019
@codecov

This comment has been minimized.

Copy link

commented Jun 27, 2019

Codecov Report

Merging #247 into develop will increase coverage by 0.06%.
The diff coverage is 86.95%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #247      +/-   ##
===========================================
+ Coverage    79.58%   79.65%   +0.06%     
===========================================
  Files           21       21              
  Lines         1406     1440      +34     
  Branches       260      263       +3     
===========================================
+ Hits          1119     1147      +28     
- Misses         211      217       +6     
  Partials        76       76
Impacted Files Coverage Δ
hub/src/server/drivers/diskDriver.ts 86.11% <100%> (ø) ⬆️
hub/src/server/drivers/GcDriver.ts 91.93% <100%> (ø) ⬆️
hub/src/server/http.ts 72.44% <84.61%> (+1.68%) ⬆️
hub/src/server/utils.ts 91.66% <87.5%> (-2.62%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe14a8b...a7f04da. Read the comment docs.

@zone117x zone117x changed the title Implement mutex around http endpoints for file mutations Implement mutex around file mutation http handlers Jun 28, 2019

export class AsyncMutexScope {

private readonly _opened: Map<string, AsyncMutex> = new Map()

This comment has been minimized.

Copy link
@kantai

kantai Jun 28, 2019

Member

Why use AsyncMutex instead of using a Set<string> that only stores values if the mutex is currently held?

This comment has been minimized.

Copy link
@zone117x

zone117x Jun 28, 2019

Author Member

AsyncMutex is just a lightweight placeholder object with possibly useful fields (currently not used since everything is handled in the tryAcquire method). Also thought Map was more performant than Set<T> but looks like that is not the case. Will simplify 👍

This comment has been minimized.

Copy link
@zone117x

zone117x Jun 28, 2019

Author Member

Addressed in 7899979

hub/src/server/http.ts Outdated Show resolved Hide resolved
@kantai

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

I think we should update the README file describing the driver interface with information on the mutexes -- in particular that a driver can expect that each of its functions will be mutexed by path.

Copy link
Member

left a comment

This all looks good to me, though I think that you should update the README description of the driver interface.

zone117x added 2 commits Jul 23, 2019
* feature/read-file:
  Add docs for performRead to driver model definition in README
  Add docs for performRename and performStat to driver model definition in README
  Update blockstack.js dep out of beta version
  Implement decodeTokenForPayload helper function
  Update to blockstack.js beta package to fix typing collisions with bitcoinjs-lib v5
  Add tests for schema validation warnings
  Add additional config param schema descriptions, cleanup sample configs
  Config schema validation that logs console warnings
@zone117x

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

@kantai Readme for drivel model updated with all new interface functions and note about path mutex.
Ping @jcnelson for review.

README.md Outdated Show resolved Hide resolved
hub/src/server/http.ts Outdated Show resolved Hide resolved
Copy link
Member

left a comment

This more or less looks good, but I just wanted some clarity on two points:

  • Whether or not we need to guarantee mutual exclusion for other POST paths, like list-files
  • Whether or not Node.js's coroutine implementation allows for a schedule that leads to a violation of mutual exclusion, given the mutex implementation
zone117x added 3 commits Jul 26, 2019
* feature/file-rename:
  Use single `topLevelStorage` arg in file rename
…tation
@zone117x zone117x requested review from jcnelson and kantai Jul 26, 2019
@moxiegirl moxiegirl referenced this pull request Jul 29, 2019
Copy link
Member

left a comment

This LGTM, but please address my only comment about the ordering of this._opened.add(id) and spawnOwner() in the mutex before merging.

* develop:
@zone117x zone117x changed the base branch from feature/read-file to develop Jul 30, 2019
…rant locking issues
@kantai
kantai approved these changes Jul 30, 2019
Copy link
Member

left a comment

Looks good to me! Thanks @zone117x

@zone117x zone117x merged commit a5dcfbd into develop Jul 30, 2019
3 checks passed
3 checks passed
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 79.65% (+0.06%) compared to 5d6ca49
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.