Skip to content

[Issue] Add encoding to file lock names #40335

@m2-assistant

Description

@m2-assistant

This issue is automatically created based on existing pull request: #40330: Add encoding to file lock names


Description (*)

This pull request was created to address for Magento instances using the file lock provider. If a lock name contains characters that are forbidden for file names (e.g., / on Unix-like systems), the operating system will fail to create the lock file, leading to a FileSystemException being thrown.

There are instances where the system attempts to acquire a lock using a string that can contain these forbidden characters. An example includes: app/code/Magento/Catalog/Plugin/ProductRepositorySaveOperationSynchronizer.php::aroundSave(). This plugin wraps calls to product saves in a ProductMutex, which is implemented using a lock, and the name of that lock is determined by the product SKU. The product SKU attribute has no restriction that makes the / character forbidden.

To address this limitation, I have introduced file lock name encoding. I chose rawurlencode() for a few reasons:

  • It is an injective algorithm, so naming collisions are impossible.

  • It is human readable, so users that wish to monitor the lock directory have a relatively unchanged experience.

  • Easy to decode using rawurldecode().

  • Encodes forbidden characters on Unix-like operating systems, as well as Windows.

    • Forbidden Linux/Unix Characters:
      • /
    • Forbidden Windows Characters:
      • <
      • >
      • :
      • "
      • /
      • \
      • |
      • ?
      • *

    % is valid for both types of systems.

Implementation details

  1. The lock name is encoded using rawurlencode() within Magento\Framework\Lock\Backend\FileLock::getLockPath().
  2. The corresponding file path generation is automatically handled in lock(), unlock(), and isLocked() methods because they all utilize the updated getLockPath() method.
  3. The cleanupOldLocks() method was also updated to use rawurldecode() before checking lock status, preventing issues where double-encoded paths would prevent detection of active locks or deletion of encoded lock files.

Manual testing scenarios (*)

  1. Switch to the file lock provider:
bin/magento setup:config:set --lock-provider=file --lock-file-path=var/locks
bin/magento cache:flush
  1. Acquire an admin access token:
export MAGENTO_TOKEN=$(curl -k -s \
  -X POST "https://magento.test/rest/V1/integration/admin/token" \
  -H "Content-Type: application/json" \
  -d '{
    "username": "john.smith", 
    "password": "password123"
  }' | tr -d '"')
echo "Token Acquired: $MAGENTO_TOKEN"
  1. Using the token acquired above, make a POST request to the products endpoint. In the request body, ensure that a / is present in the sku attribute:
curl -k \
  -X POST "https://magento.test/rest/V1/products" \
  -H "Authorization: Bearer $MAGENTO_TOKEN" \
  -H "Content-Type: application/json" \
  -d '{
      "product": {
          "sku": "ABC/XYZ",
          "name": "A",
          "attribute_set_id": 4,
          "price": 5.00,
          "status": 1,
          "visibility": 4,
          "type_id": "simple"
      }
  }'
  1. Observe that an exception is returned with the following message: "Cannot acquire a lock.", returning a status of 400.
  2. Apply PR, and repeat steps 1-3.
  3. Observe that the expected product JSON is returned, returning a status of 200 (success).

Questions or comments

I acknowledge that rawurlencode() might not be the optimal encoding mechanism in this scenario. I would argue it is good for this scenario, but I do not claim to be an expert on methods of encoding text. If anyone has a better idea, or concerns with this encoding method, please feel free to let me know!

Per the Adobe Commerce documentation, Windows is not supported, but I am not 100% sure if this applies to new versions of Magento Open Source as well. If Windows is not supported, I could potentially devise a more simple encoding scheme that just handles /. For extra clarification, I did all development & testing on Linux.

I am a bit concerned with how backwards compatible this change might be. If the deployment occurs while a long-lasting lock is active, the new code will miss the existing lock if it's encoded name is different from it's current name.

  • If Windows support can be ignored, I can simplify the encoding, making this scenario impossible.
  • If Windows support is required, I might need some guidance on this point in particular.

Finally, this is my first contribution to Magento Open Source. I did my best to follow the contribution guidelines, but it's very possible I did something wrong. Please let me know so I can do better next time.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
  • All automated tests passed successfully (all builds are green)

Metadata

Metadata

Assignees

Type

No type

Projects

Status

Ready for Confirmation

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions