feat(arcor2_arserver): accept attempts to write-lock already locked s…

…tuff

src/python/arcor2_arserver/CHANGELOG.md

@@ -7,6 +7,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 ### Changed
 
 - Updated dependencies, switched to Python 3.11.
+- `WriteLock` RPC now does not return an error when a user tries to write-lock something already locked (by the same user).
 
 ## [1.1.1] - 2024-01-23
 

src/python/arcor2_arserver/lock/lock.py

@@ -580,7 +580,7 @@ def check_children(_root: str, obj_ids: set[str], read: bool = False) -> bool:
                 if read:
                     if owner not in lock_item.read[_obj_id]:
                         return False
-                    if lock_item.read[_obj_id].count != 1:
+                    if len(lock_item.read[_obj_id]) != 1:
                         return False
                 else:
                     if owner != lock_item.write[_obj_id]:

src/python/arcor2_arserver/lock/structures.py

@@ -21,7 +21,7 @@ class LockedObject:
 
     def __init__(self) -> None:
         # object id: owners
-        self.read: dict[str, list[str]] = {}
+        self.read: dict[str, set[str]] = {}
         self.write: dict[str, str] = {}
 
         self.tree: bool = False
@@ -36,9 +36,9 @@ def read_lock(self, obj_id: str, owner: str) -> bool:
             return False
 
         if obj_id in self.read:  # already locked
-            self.read[obj_id].append(owner)
+            self.read[obj_id].add(owner)
         else:
-            self.read[obj_id] = [owner]
+            self.read[obj_id] = {owner}
         return True
 
     def read_unlock(self, obj_id: str, owner: str) -> None:
@@ -51,9 +51,9 @@ def read_unlock(self, obj_id: str, owner: str) -> None:
         if owner not in self.read[obj_id]:
             raise CannotUnlock(f"{obj_id} lock is not owned by {owner}.")
 
-        if len(self.read[obj_id]) > 1:
-            self.read[obj_id].remove(owner)
-        else:
+        self.read[obj_id].remove(owner)
+
+        if not self.read[obj_id]:
             del self.read[obj_id]
 
     def write_lock(self, obj_id: str, owner: str, lock_tree: bool) -> bool:

src/python/arcor2_arserver/rpc/lock.py

@@ -1,12 +1,19 @@
 from websockets.server import WebSocketServerProtocol as WsClient
 
 from arcor2_arserver import globals as glob
+from arcor2_arserver import logger
 from arcor2_arserver.lock.exceptions import CannotLock
 from arcor2_arserver_data import rpc as srpc
 
 
 async def write_lock_cb(req: srpc.lock.WriteLock.Request, ui: WsClient) -> None:
-    if not await glob.LOCK.write_lock(req.args.object_id, glob.USERS.user_name(ui), req.args.lock_tree, notify=True):
+    user_name = glob.USERS.user_name(ui)
+
+    if await glob.LOCK.is_write_locked(req.args.object_id, user_name, req.args.lock_tree):
+        logger.warn(f"User {user_name} attempted to re-acquire lock for {req.args.object_id}. Pretending it is OK...")
+        return
+
+    if not await glob.LOCK.write_lock(req.args.object_id, user_name, req.args.lock_tree, notify=True):
         raise CannotLock(glob.LOCK.ErrMessages.LOCK_FAIL.value)
 
 

src/python/arcor2_arserver/tests/test_lock.py

@@ -72,7 +72,10 @@ async def patch() -> set[str]:
 
     assert await glob.LOCK.get_locked_roots_count() == 0
 
-    await glob.LOCK.write_lock(ap_ap.id, user, True)
+    assert await glob.LOCK.write_lock(ap_ap.id, user, True)
+
+    # attempt to lock it again should be ok
+    # assert await glob.LOCK.write_lock(ap_ap.id, user, True)
 
     assert await glob.LOCK.is_write_locked(test_object.id, user)
     assert await glob.LOCK.is_write_locked(ap.id, user)
@@ -94,12 +97,12 @@ async def test_base_logic(lock: Lock) -> None:
     ap = next(ap for ap in lock.project.action_points if ap.name == "ap")
 
     assert not lock._locked_objects
-    await lock.write_lock(ap.id, test)
+    assert await lock.write_lock(ap.id, test)
     assert ap.id in lock._locked_objects
     assert not await lock.read_lock(ap.id, "second_user")
     await lock.write_unlock(ap.id, test)
 
-    await lock.write_lock(ap.id, test, True)
+    assert await lock.write_lock(ap.id, test, True)
     assert not await lock.read_lock(ap.id, test)
     await lock.write_unlock(ap.id, test)
     assert not lock._locked_objects
@@ -132,7 +135,7 @@ async def test_locking_special_names(lock: Lock) -> None:
         await lock.write_lock(lock.Owners.SERVER, test)
 
     for special_id in lock.SpecialValues:
-        await lock.read_lock(special_id, test)
+        assert await lock.read_lock(special_id, test)
         assert special_id in lock._locked_objects
         assert special_id in lock._locked_objects[special_id].read
         await lock.read_unlock(special_id, test)
@@ -205,25 +208,25 @@ async def test_recursive_locking(lock: Lock) -> None:
     test = "test"
 
     assert not await lock.is_write_locked(lock.scene.id, test)
-    await lock.write_lock(lock.scene.id, test)
+    assert await lock.write_lock(lock.scene.id, test)
     assert await lock.is_write_locked(lock.scene.id, test)
     assert not await lock.is_read_locked(lock.scene.id, test)
     assert not await lock.write_lock(lock.scene.id, test)
     await lock.write_unlock(lock.scene.id, test)
+    assert not await lock.is_write_locked(lock.scene.id, test)
 
     assert not await lock.is_read_locked(lock.scene.id, test)
-    await lock.read_lock(lock.scene.id, test)
+    assert await lock.read_lock(lock.scene.id, test)
     assert await lock.is_read_locked(lock.scene.id, test)
     assert not await lock.is_write_locked(lock.scene.id, test)
-    assert await lock.read_lock(lock.scene.id, test)
-    await lock.read_unlock(lock.scene.id, test)
+    assert await lock.read_lock(lock.scene.id, test)  # attempt to get the lock again should be ok
     assert not await lock.write_lock(lock.scene.id, test)
     await lock.read_unlock(lock.scene.id, test)
 
     # Cover negative return when trying to lock tree with something already locked
     ap = next(ap for ap in lock.project.action_points if ap.name == "ap")
     ap_ap = next(ap for ap in lock.project.action_points if ap.name == "ap_ap")
-    await lock.write_lock(ap_ap.id, test)
+    assert await lock.write_lock(ap_ap.id, test)
     assert not await lock.write_lock(ap.id, test, True)
 
 
@@ -236,7 +239,7 @@ async def test_count_methods(lock: Lock) -> None:
 
     assert await lock.get_locked_roots_count() == 0
     assert await lock.get_write_locks_count() == 0
-    await lock.write_lock(lock.scene.id, test)
+    assert await lock.write_lock(lock.scene.id, test)
     assert await lock.get_locked_roots_count() == 1
     assert await lock.get_write_locks_count() == 1
     await lock.write_unlock(lock.scene.id, test)
@@ -255,7 +258,7 @@ async def test_notification_queue(lock: Lock) -> None:
     action = lock.project.ap_actions(ap_ap_ap.id)[0]
 
     # test check remove of implicitly locked child
-    await lock.write_lock(ap_ap_ap.id, test, True, True)
+    assert await lock.write_lock(ap_ap_ap.id, test, True, True)
     assert lock.all_ui_locks
     await check_notification_content(lock, test, tree_ap_ids + [ori.id, action.id])
     await lock.write_unlock(ap_ap_ap.id, test, True)
@@ -271,14 +274,14 @@ async def test_possibility_of_locking_tree(lock: Lock) -> None:
     ap = next(ap for ap in lock.project.action_points if ap.name == "ap")
     ap_ap = next(ap for ap in lock.project.action_points if ap.name == "ap_ap")
 
-    await lock.write_lock(ap.id, test)
+    assert await lock.write_lock(ap.id, test)
     with pytest.raises(CannotLock):
         await lock.check_lock_tree(ap_ap.id, "another_user")
     await lock.check_lock_tree(ap_ap.id, test)
     await lock.write_unlock(ap.id, test)
     await lock.check_lock_tree(ap_ap.id, test)
 
-    await lock.read_lock(ap.id, test)
+    assert await lock.read_lock(ap.id, test)
     with pytest.raises(CannotLock):
         await lock.check_lock_tree(ap_ap.id, "another_user")
     await lock.check_lock_tree(ap_ap.id, test)
@@ -297,7 +300,7 @@ async def test_updating_lock(lock: Lock) -> None:
     with pytest.raises(LockingException):
         await lock.update_lock(ap.id, test, rpc.lock.UpdateType.TREE)
 
-    await lock.write_lock([ap.id, ap_ap.id], test)
+    assert await lock.write_lock([ap.id, ap_ap.id], test)
     with pytest.raises(LockingException):
         await lock.update_lock(ap.id, test, rpc.lock.UpdateType.TREE)
     await lock.write_unlock(ap_ap.id, test)
@@ -353,12 +356,12 @@ async def test_auto_release(lock: Lock) -> None:
 
     # Test auto-release of locks and auto locking of child in tree
     lock._lock_timeout = 2
-    await lock.write_lock(ap.id, test, True, True)
+    assert await lock.write_lock(ap.id, test, True, True)
     assert await lock.is_write_locked(ap_ap_ap.id, test)
     assert await lock.is_write_locked(ap_ap.id, test)
     await check_notification_content(lock, test, [ap.id, ap_ap.id, ap_ap_ap.id, ori.id, action.id])
 
-    await lock.read_lock(ap2.id, test)
+    assert await lock.read_lock(ap2.id, test)
     await lock.schedule_auto_release(test)
     await lock.cancel_auto_release(test)
     assert await lock.is_write_locked(ap.id, test)
@@ -411,7 +414,7 @@ async def test_update_parent(lock: Lock) -> None:
     ap_ap = next(ap for ap in lock.project.action_points if ap.name == "ap_ap")
     ap2 = next(ap for ap in lock.project.action_points if ap.name == "ap2")
 
-    await lock.write_lock([ap_ap.id, ap.id, ap2.id], test)
+    assert await lock.write_lock([ap_ap.id, ap.id, ap2.id], test)
     ap_ap.parent = ap2.id
     await lock.update_write_lock(ap_ap.id, ap.id, test)
     assert await lock.is_write_locked(ap_ap.id, test)
@@ -432,7 +435,7 @@ async def test_check_remove(lock: Lock) -> None:
     action = lock.project.ap_actions(ap_ap_ap.id)[0]
 
     # test check remove of implicitly locked child
-    await lock.write_lock(ap_ap.id, test, True, True)
+    assert await lock.write_lock(ap_ap.id, test, True, True)
     assert not await lock.check_remove(ap.id, "second_user")
     assert not await lock.check_remove(ap_ap_ap.id, "second_user")
     assert not await lock.check_remove(ori.id, "second_user")
@@ -445,11 +448,11 @@ async def test_check_remove(lock: Lock) -> None:
 
     # test check remove when some child locked
     for obj_id in (action.id, ori.id):
-        await lock.read_lock(obj_id, test)
+        assert await lock.read_lock(obj_id, test)
         assert not await lock.check_remove(ap.id, "second_user")
         await lock.read_unlock(obj_id, test)
 
-        await lock.write_lock(obj_id, test)
+        assert await lock.write_lock(obj_id, test)
         assert not await lock.check_remove(ap.id, "second_user")
         await lock.write_unlock(obj_id, test)
     assert not await lock.check_remove(ap.id, test)
@@ -458,7 +461,7 @@ async def test_check_remove(lock: Lock) -> None:
     await lock.read_lock(ap.id, "second_user")
     await lock.read_lock(ap.id, "third_user")
     assert not await lock.check_remove(ap.id, test)
-    await lock.read_lock(ap.id, test)
+    assert await lock.read_lock(ap.id, test)
     assert not await lock.check_remove(ap.id, test)
 
 
@@ -520,6 +523,9 @@ def test_lock_events(start_processes: None, ars: ARServer, scene: cmn.Scene, pro
 
     lock_object(ars, ap.id, True)
 
+    # second attempt to call RPC should produce warning, return True and no events...
+    lock_object(ars, ap.id, True, expect_event=False)
+
     # lock object and expect event about it on newly logged UI
     ars2 = ARServer(ars_connection_str(), timeout=30, event_mapping=event_mapping)
     event(ars2, events.p.OpenProject)

src/python/arcor2_arserver/tests/testutils.py

@@ -224,13 +224,14 @@ def close_project(ars: ARServer) -> None:
     event(ars, events.p.ProjectClosed)
 
 
-def lock_object(ars: ARServer, obj_id: str, lock_tree: bool = False) -> None:
+def lock_object(ars: ARServer, obj_id: str, lock_tree: bool = False, expect_event: bool = True) -> None:
     assert ars.call_rpc(
         rpc.lock.WriteLock.Request(get_id(), rpc.lock.WriteLock.Request.Args(obj_id, lock_tree)),
         rpc.lock.WriteLock.Response,
     ).result
 
-    event(ars, events.lk.ObjectsLocked)
+    if expect_event:
+        event(ars, events.lk.ObjectsLocked)
 
 
 def unlock_object(ars: ARServer, obj_id: str) -> None: