Feature 941

[anushka-singh]

Feature or Bugfix

• Feature

Detail

Share requests running in parallel override one another.

Relates

• Two share requests running in parallel can override each other #941

Testing

• 2 simultaneous shares got processed successfully with locking mechanism.
• Here is what the dataset_lock DB looks like and the values in it.

- acquiredBy column show the share which last acquired the lock for the particular dataset

Security

Please answer the questions below briefly where applicable, or write N/A. Based on
OWASP 10.

• Does this PR introduce or modify any input fields or queries - this includes
  fetching data from storage outside the application (e.g. a database, an S3 bucket)?
  • Is the input sanitized?
  • What precautions are you taking before deserializing the data you consume?
  • Is injection prevented by parametrizing queries?
  • Have you ensured no eval or similar functions are used?
• Does this PR introduce any functionality or component that requires authorization?
  • How have you ensured it respects the existing AuthN/AuthZ mechanisms?
  • Are you logging failed auth attempts?
• Are you using or adding any cryptographic features?
  • Do you use a standard proven implementations?
  • Are the used keys controlled by the customer? Where are they stored?
• Are you introducing any new policies/roles/users?
  • Have you used the least-privilege principle? How?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[anushka-singh]

Filed another issue for opening a discussion on another locking usecase: #1074.

[anushka-singh]

Alembic script failing with :

ERROR [alembic.util.messaging] Multiple head revisions are present for given argument 'head'; please specify a specific target revision, '<branchname>@head' to narrow to a specific head, or 'heads' for all heads
FAILED: Multiple head revisions are present for given argument 'head'; please specify a specific target revision, '<branchname>@head' to narrow to a specific head, or 'heads' for all heads

I am not sure how to specify which head version alembic should run upgrade with in the script. @noah-paige Can you suggest?

[noah-paige]

Alembic script failing with :

ERROR [alembic.util.messaging] Multiple head revisions are present for given argument 'head'; please specify a specific target revision, '<branchname>@head' to narrow to a specific head, or 'heads' for all heads
FAILED: Multiple head revisions are present for given argument 'head'; please specify a specific target revision, '<branchname>@head' to narrow to a specific head, or 'heads' for all heads

I am not sure how to specify which head version alembic should run upgrade with in the script. @noah-paige Can you suggest?

@anushka-singh - I believe it is because there are 2 alembic migration scripts building off of the same down_revision

A PR we merged last week has revision id of 6c9a8afee4e4 which should be the latest in OS and the migration script in this PR should be updated to have that revision id as its down revision

[noah-paige]

@anushka-singh - sorry for all the back and forth ...

with the latest changes to add .with_for_update().one() I am now receiving error: "Arguments: (AttributeError("'DatasetLock' object has no attribute 'update'"),)"

I think for we have 2 options:

• (1) For def release_lock() we can handle the update the same as for acquire lock

            dataset_lock = (
                session.query(DatasetLock)
                .filter(
                    and_(
                        DatasetLock.datasetUri == dataset_uri,
                        DatasetLock.isLocked == True,
                        DatasetLock.acquiredBy == share_uri
                    )
                )
                .with_for_update().one()
            )
           if dataset_lock:
                dataset_lock.isLocked = False
                dataset_lock.acquiredBy = ''
...

• (2) OR perform query and update all in 1:

            query = (
                session.query(DatasetLock)
                .filter(
                    and_(
                        DatasetLock.datasetUri == dataset_uri,
                        DatasetLock.isLocked == True,
                        DatasetLock.acquiredBy == share_uri
                    )
                )
                .update(
                {
                    "isLocked": False,
                    "acquiredBy": ''
                },
                )
            )
``` (and likely can handle `acquire_lock()` similarly)

[anushka-singh]

Hmm, that is really weird. I did not come across this issue in my testing!

But I have pushed out another revision with solution (1). Please LMK if its fixed now. I will also try it out again on my end.

[noah-paige]

will do - trying again now

[noah-paige]

One (..hopefully last) issue I am running into with def acquire_lock() (and potentially def release_lock()) - When running multiple shares all at once the ones that need to wait to acquire lock run into an error similar to below once beginning to process the share:
sqlalchemy.orm.exc.DetachedInstanceError: Instance <Dataset at 0x7f8f546c4190> is not bound to a Session; attribute refresh operation cannot proceed (Background on this error at: http://sqlalche.me/e/13/bhk3)

I believe this is because one the first iteration when the share tries to acquire the lock and fails - since we are using the .one() and not .first() (or .one_or_none()) for the query, we get an error sqlalchemy.orm.exc.NoResultFound and thus go to the excpetion block and run session.rollback()

Now session.rollback() closes the session and makes the previous objects retrieved from the session stale and "boundless" (i.e. dataset, share object, etc. all from get_share_data() and get_share_data_items())

To resolve we have a couple of options:

• (1) use .first() or similar in the query so dataset_lock can be None and we do not always go to exception block if no results found
• (2) use session.expunge_all() before we run session.rollback() (definitely required for acquire_lock() - maybe not for release_lock() as we are done processing share at this point)

I would think best if we implement both 1 and 2 above - should both be quick changes :)

Also please let me know if you are experiencing similar

[anushka-singh]

One (..hopefully last) issue I am running into with def acquire_lock() (and potentially def release_lock()) - When running multiple shares all at once the ones that need to wait to acquire lock run into an error similar to below once beginning to process the share: sqlalchemy.orm.exc.DetachedInstanceError: Instance <Dataset at 0x7f8f546c4190> is not bound to a Session; attribute refresh operation cannot proceed (Background on this error at: http://sqlalche.me/e/13/bhk3)

I believe this is because one the first iteration when the share tries to acquire the lock and fails - since we are using the .one() and not .first() (or .one_or_none()) for the query, we get an error sqlalchemy.orm.exc.NoResultFound and thus go to the excpetion block and run session.rollback()

Now session.rollback() closes the session and makes the previous objects retrieved from the session stale and "boundless" (i.e. dataset, share object, etc. all from get_share_data() and get_share_data_items())

To resolve we have a couple of options:

• (1) use .first() or similar in the query so dataset_lock can be None and we do not always go to exception block if no results found
• (2) use session.expunge_all() before we run session.rollback() (definitely required for acquire_lock() - maybe not for release_lock() as we are done processing share at this point)

I would think best if we implement both 1 and 2 above - should both be quick changes :)

Also please let me know if you are experiencing similar

In my testing, I have not come across this issue. I am not sure why you are facing this..

I have implemented both 1 and 2 solutions.
Implemented session.expunge_all() in release_lock as well to add guardrails.

[noah-paige]

Tested in AWS for approving and revoking 10+ shares at the same time - all works perfectly, thanks for your contribution @anushka-singh