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

feat(payments): add api locking for payments core #1898

Merged
merged 26 commits into from
Sep 25, 2023
Merged

Conversation

Abhicodes-crypto
Copy link
Contributor

@Abhicodes-crypto Abhicodes-crypto commented Aug 8, 2023

Type of Change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring
  • Dependency updates
  • Documentation
  • CI/CD

Description

This PR introduces an API locking framework and uses it with the core payments flow. For now both are under the locking feature flag. After thorough testing they can be moved under release feature.
Problem statement:

  • When webhooks, sync, redirection happen almost at the same time there could be race conditions and inconsistency in the status of the payment.
  • What if capture and cancel happens at the same time.

API locking prevents that by locking the flow.

  • Another use case of API locking, while attempting to not send duplicate webhooks to merchant, the logic was to fetch a table from DB and then check a value if it is false send the webhook or else don't send it . But if some other request changes the value to true after the previous request fetches it. Here race condition could happen and multiple webhooks could still be done. So API locking can solve this.

TODO :

  • Add status validations.
  • Speculate what action should be taken in case of validation fails.
  • Add logs.
  • Add metrics
  • Remove hard coded values and take config from Redis.
  • What action to take when the retries are exhausted.
  • Refactor such that the get_key_and_lock_resource function returns only the status Acquired or gives error and only over acquired status release_lock is implemented.
  • Code clean up

Additional Changes

  • This PR modifies the API contract
  • This PR modifies the database schema
  • This PR modifies application configuration/environment variables

Motivation and Context

How did you test it?

Monitored redis if the locks are getting created or not.
image

Webhooks testing still in progress

Checklist

  • I formatted the code cargo +nightly fmt --all
  • I addressed lints thrown by cargo clippy
  • I reviewed submitted code
  • I added unit tests for my changes where possible
  • I added a CHANGELOG entry if applicable

@Abhicodes-crypto Abhicodes-crypto added A-core Area: Core flows A-payments Area: payments labels Aug 8, 2023
@Abhicodes-crypto Abhicodes-crypto self-assigned this Aug 8, 2023
@Abhicodes-crypto Abhicodes-crypto requested review from a team as code owners August 8, 2023 09:36
@Abhicodes-crypto Abhicodes-crypto marked this pull request as draft August 8, 2023 09:36
crates/router/Cargo.toml Outdated Show resolved Hide resolved
crates/router/src/core/locking.rs Outdated Show resolved Hide resolved
crates/router/src/core/locking.rs Outdated Show resolved Hide resolved
crates/router/src/core/payments.rs Outdated Show resolved Hide resolved
crates/router/src/core/payments/operations.rs Outdated Show resolved Hide resolved
crates/router/src/core/locking.rs Outdated Show resolved Hide resolved
crates/router/src/core/locking.rs Outdated Show resolved Hide resolved
crates/api_models/src/payments.rs Outdated Show resolved Hide resolved
crates/router/src/core/payments.rs Outdated Show resolved Hide resolved
crates/router/src/core/locking.rs Outdated Show resolved Hide resolved
crates/router/src/core/locking.rs Outdated Show resolved Hide resolved
crates/router/src/core/locking.rs Outdated Show resolved Hide resolved
crates/router/src/core/locking.rs Outdated Show resolved Hide resolved
crates/router/src/core/locking.rs Outdated Show resolved Hide resolved
crates/router/src/core/locking.rs Outdated Show resolved Hide resolved
crates/router/src/core/payments.rs Outdated Show resolved Hide resolved
crates/router/src/core/payments.rs Outdated Show resolved Hide resolved
crates/router/src/core/payments/operations.rs Outdated Show resolved Hide resolved
crates/router/src/core/api_locking.rs Outdated Show resolved Hide resolved
crates/router/src/core/api_locking.rs Show resolved Hide resolved
@bernard-eugine bernard-eugine added the ageing >2weeks Created > 2 weeks label Sep 1, 2023
@Abhicodes-crypto Abhicodes-crypto marked this pull request as draft September 20, 2023 15:03
@Abhicodes-crypto Abhicodes-crypto marked this pull request as ready for review September 21, 2023 09:32
inventvenkat
inventvenkat previously approved these changes Sep 22, 2023
@inventvenkat inventvenkat added this pull request to the merge queue Sep 25, 2023
Merged via the queue into main with commit 5d66156 Sep 25, 2023
10 checks passed
@inventvenkat inventvenkat deleted the api-locking branch September 25, 2023 10:32
@shakthi-22 shakthi-22 added this to the September 2023 Milestone milestone Sep 25, 2023
@SanchithHegde SanchithHegde removed the ageing >2weeks Created > 2 weeks label Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-core Area: Core flows A-payments Area: payments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants