Commit cff0c64
authored
Manage snapshots mutability issue (#2431)
<!--
Thanks for opening a pull request!
-->
<!-- In the case this PR will resolve an issue, please replace
${GITHUB_ISSUE_ID} below with the actual Github issue id. -->
Related to #2409, and partially closes #2427
# Rationale for this change
This PR fixes a thread safety issue in the `ManageSnapshots` class
similar to the one identified in `ExpireSnapshots` (#2409). While the
original issue specifically mentioned `ExpireSnapshots`, the same thread
safety vulnerability exists in `ManageSnapshots` due to identical
problematic design patterns. The same testing methodology used in #2430
was adapted for this.
**Root Cause:**
The `ManageSnapshots` class had class-level attributes (`_updates`,
`_requirements`) that were shared across all instances. When multiple
threads created different `ManageSnapshots` instances for concurrent
operations, they all shared the same underlying tuple objects for
tracking updates and requirements.
**Potential Impact:**
- Thread 1: `table1.manage_snapshots().create_tag(...)` adds updates to
shared tuple
- Thread 2: `table2.manage_snapshots().create_branch(...)` adds updates
to same shared tuple
- Result: Both threads would have mixed updates, potentially causing
incorrect operations or failures
**Solution:**
Applied the same fix as ExpireSnapshots - moved the shared class-level
attributes to instance-level attributes in the `__init__` method,
ensuring each `ManageSnapshots` instance has its own isolated state.
**Relationship to #2409:**
While #2409 specifically reported ExpireSnapshots thread safety issues,
this PR proactively addresses the same vulnerability pattern in
ManageSnapshots to prevent similar issues from occurring with snapshot
management operations (tags, branches, etc.).
## Are these changes tested?
> 📢 🔥 Big shout-out to @QlikFrederic, as the testing methodology was
largely derived from the testing and analysis done by the user! 🔥 📢
Yes, comprehensive test coverage has been added with a dedicated test
file `test_manage_snapshots_thread_safety.py`:
- **`test_manage_snapshots_thread_safety_fix()`** - Verifies that
different ManageSnapshots instances have separate update/requirement
tuples
- **`test_manage_snapshots_concurrent_operations()`** - Tests concurrent
operations don't contaminate each other
- **`test_manage_snapshots_concurrent_different_tables()`** - Tests
concurrent operations on different tables work correctly
- **`test_manage_snapshots_cross_table_isolation()`** - Validates no
cross-contamination of operations between tables
-
**`test_manage_snapshots_concurrent_same_table_different_operations()`**
- Tests concurrent operations on the same table
All tests demonstrate that the thread safety fix works correctly and
that concurrent ManageSnapshots operations maintain proper isolation.
## Are there any user-facing changes?
**No breaking changes.** The public API remains identical:
- All existing `ManageSnapshots` methods work the same way
(`create_tag`, `create_branch`, `delete_tag`, etc.)
- Method signatures are unchanged
- Behavior is identical except for the thread safety improvement
**Behavioral improvement:**
- Concurrent `manage_snapshots()` operations on different tables now
work correctly without interference
- No risk of mixed updates/requirements between different
ManageSnapshots instances in multi-threaded environments
- Improved reliability for applications using ManageSnapshots in
concurrent scenarios
This is a pure bug fix.1 parent d5e039f commit cff0c64
1 file changed
+7
-2
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
810 | 810 | | |
811 | 811 | | |
812 | 812 | | |
813 | | - | |
814 | | - | |
| 813 | + | |
| 814 | + | |
| 815 | + | |
| 816 | + | |
| 817 | + | |
| 818 | + | |
| 819 | + | |
815 | 820 | | |
816 | 821 | | |
817 | 822 | | |
| |||
0 commit comments