diff --git a/src/python/arcor2_arserver/CHANGELOG.md b/src/python/arcor2_arserver/CHANGELOG.md index f9a2c44df..3e51409df 100644 --- a/src/python/arcor2_arserver/CHANGELOG.md +++ b/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 diff --git a/src/python/arcor2_arserver/lock/lock.py b/src/python/arcor2_arserver/lock/lock.py index 8c3aa175a..8cc4bc83a 100644 --- a/src/python/arcor2_arserver/lock/lock.py +++ b/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]: diff --git a/src/python/arcor2_arserver/lock/structures.py b/src/python/arcor2_arserver/lock/structures.py index 609d221c2..dda2ae17b 100644 --- a/src/python/arcor2_arserver/lock/structures.py +++ b/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: diff --git a/src/python/arcor2_arserver/rpc/lock.py b/src/python/arcor2_arserver/rpc/lock.py index 3c0f2c32e..4373b8529 100644 --- a/src/python/arcor2_arserver/rpc/lock.py +++ b/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) diff --git a/src/python/arcor2_arserver/tests/test_lock.py b/src/python/arcor2_arserver/tests/test_lock.py index 7f6b3ffb1..14f8f3cde 100644 --- a/src/python/arcor2_arserver/tests/test_lock.py +++ b/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) diff --git a/src/python/arcor2_arserver/tests/testutils.py b/src/python/arcor2_arserver/tests/testutils.py index 2718c8254..4da143bab 100644 --- a/src/python/arcor2_arserver/tests/testutils.py +++ b/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: