Copilot commented on code in PR #8217: URL: https://github.com/apache/incubator-seata/pull/8217#discussion_r4023138576
########## server/src/main/java/org/apache/seata/server/storage/file/lock/DefaultFileLockStore.java: ########## @@ -0,0 +1,68 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.seata.server.storage.file.lock; + +import org.apache.seata.core.exception.TransactionException; +import org.apache.seata.core.lock.Locker; +import org.apache.seata.server.session.BranchSession; +import org.apache.seata.server.session.GlobalSession; +import org.apache.seata.server.storage.file.spi.FileLockStore; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.slf4j.MDC; + +import static org.apache.seata.core.context.RootContext.MDC_KEY_BRANCH_ID; + +/** + * In-memory file lock store. + */ +public class DefaultFileLockStore implements FileLockStore { + + private static final Logger LOGGER = LoggerFactory.getLogger(DefaultFileLockStore.class); + + @Override + public Locker getLocker(BranchSession branchSession) { + return new FileLocker(branchSession); + } + + @Override + public boolean releaseBranchLock(BranchSession branchSession) throws TransactionException { + if (branchSession == null) { + throw new IllegalArgumentException("branchSession can't be null for memory/file locker."); + } + try { + return new FileLocker(branchSession).releaseLock(); + } catch (Exception t) { + LOGGER.error("unLock error, branchSession:{}", branchSession, t); + return false; + } + } + + @Override + public boolean releaseGlobalLock(GlobalSession globalSession) throws TransactionException { + boolean releaseLockResult = true; + for (BranchSession branchSession : globalSession.getBranchSessions()) { + try { + MDC.put(MDC_KEY_BRANCH_ID, String.valueOf(branchSession.getBranchId())); + releaseLockResult = releaseBranchLock(branchSession); Review Comment: This overwrites an earlier failure with each branch's result, so a failed release followed by a successful one returns `true` even though the global session still owns locks. Aggregate with non-short-circuiting AND so every branch is attempted but any failure is preserved. ########## server/src/main/java/org/apache/seata/server/session/SessionHolder.java: ########## @@ -158,15 +173,77 @@ public static void init(SessionMode sessionMode) { } } + private static FileStoreRuntime openFileStoreRuntime() { + String engineName = StoreConfig.getFileEngineName(); + FileStoreProvider provider = FileStoreProviderFactory.getProvider(engineName); + String sessionStoreDir = CONFIG.getConfig(ConfigurationKeys.STORE_FILE_DIR, DEFAULT_SESSION_STORE_FILE_DIR); + if (StringUtils.isBlank(sessionStoreDir)) { + throw new StoreException("the {store.file.dir} is empty."); + } + String sessionStorePath = sessionStoreDir + separator + XID.getPort(); + return provider.open(new FileStoreContext(ROOT_SESSION_MANAGER_NAME, Paths.get(sessionStorePath))); + } + + private static void initFileMode(FileStoreRuntime runtime) { + String vGroupMappingStoreDir = + CONFIG.getConfig(ConfigurationKeys.STORE_FILE_DIR, DEFAULT_VGROUP_MAPPING_STORE_FILE_DIR); + if (StringUtils.isBlank(vGroupMappingStoreDir)) { + throw new StoreException("the {store.file.dir} is empty."); + } + String vGroupMappingStorePath = vGroupMappingStoreDir + separator + XID.getPort(); + VGroupMappingStoreManager vGroupMappingManager = EnhancedServiceLoader.load( + VGroupMappingStoreManager.class, SessionMode.FILE.getName(), new Object[] {vGroupMappingStorePath}); + DistributedLocker distributedLocker = DistributedLockerFactory.getDistributedLocker(SessionMode.FILE.getName()); + ROOT_VGROUP_MAPPING_MANAGER = vGroupMappingManager; + DISTRIBUTED_LOCKER = distributedLocker; + ROOT_SESSION_MANAGER = runtime.sessionManager(); + runtime.recover(sessions -> reload(sessions, SessionMode.FILE, true, true)); + runtime.startBackgroundServices(); + } + + private static void rollbackFailedFileInitialization( + Throwable startupFailure, FileStoreRuntime runtime, boolean lockManagerInstalled) { + try { + if (lockManagerInstalled) { + cleanup(startupFailure, LockerManagerFactory::destroy); + cleanup(startupFailure, () -> EnhancedServiceLoader.unload(LockManager.class)); + } + cleanup(startupFailure, runtime == null ? null : runtime::close); + } finally { + clearFileModeReferences(); + } Review Comment: Cleanup runs in the opposite order for the runtime's live services: if `startBackgroundServices()` partially starts and then fails, the lock manager is removed before `runtime.close()` can stop those services. A provider background task that resolves the lock manager can therefore fail or race during rollback; close the runtime first, then uninstall its lock facade. This issue also appears on line 556 of the same file. ########## server/src/main/java/org/apache/seata/server/storage/file/lock/FileLockManager.java: ########## @@ -17,44 +17,46 @@ package org.apache.seata.server.storage.file.lock; import org.apache.seata.common.loader.LoadLevel; +import org.apache.seata.common.loader.Scope; import org.apache.seata.core.exception.TransactionException; import org.apache.seata.core.lock.Locker; import org.apache.seata.server.lock.AbstractLockManager; import org.apache.seata.server.session.BranchSession; import org.apache.seata.server.session.GlobalSession; -import org.apache.seata.server.storage.raft.lock.RaftLockManager; -import org.slf4j.MDC; - -import java.util.List; - -import static org.apache.seata.core.context.RootContext.MDC_KEY_BRANCH_ID; +import org.apache.seata.server.storage.file.spi.FileLockStore; /** * The type file lock manager. * */ -@LoadLevel(name = "file") +@LoadLevel(name = "file", scope = Scope.PROTOTYPE) public class FileLockManager extends AbstractLockManager { + private final FileLockStore lockStore; + + protected FileLockManager() { + this(new DefaultFileLockStore()); + } + + public FileLockManager(FileLockStore lockStore) { + if (lockStore == null) { + throw new IllegalArgumentException("lockStore must not be null"); + } + this.lockStore = lockStore; + } + @Override public Locker getLocker(BranchSession branchSession) { - return new FileLocker(branchSession); + return lockStore.getLocker(branchSession); Review Comment: The inherited manager-wide operations (`isLockable`, `cleanAllLocks`, and `updateLockStatus`) call `getLocker()` in `AbstractLockManager`, which dispatches here with a null `BranchSession`. The new SPI contract neither documents nor models that null call, so a valid-looking provider that requires the documented supplied session will fail on normal lock queries or cleanup. Add explicit manager-wide operations/accessor to `FileLockStore`, or make and test the nullable contract. ########## server/src/test/java/org/apache/seata/server/storage/file/lock/FileLockManagerStrategyTest.java: ########## @@ -0,0 +1,186 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.seata.server.storage.file.lock; + +import org.apache.seata.common.Constants; +import org.apache.seata.common.holder.ObjectHolder; +import org.apache.seata.common.store.LockMode; +import org.apache.seata.config.ConfigurationCache; +import org.apache.seata.core.model.BranchStatus; +import org.apache.seata.core.model.BranchType; +import org.apache.seata.core.model.LockStatus; +import org.apache.seata.server.lock.LockerManagerFactory; +import org.apache.seata.server.session.BranchSession; +import org.apache.seata.server.session.GlobalSession; +import org.apache.seata.server.storage.file.spi.FileLockStore; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.mock.env.MockEnvironment; + +import java.lang.reflect.Field; +import java.util.Map; +import java.util.Set; + +class FileLockManagerStrategyTest { + + private Object originalEnvironment; + + @BeforeEach + void beforeEach() { + originalEnvironment = ObjectHolder.INSTANCE.getObject(Constants.OBJECT_KEY_SPRING_CONFIGURABLE_ENVIRONMENT); + ObjectHolder.INSTANCE.setObject(Constants.OBJECT_KEY_SPRING_CONFIGURABLE_ENVIRONMENT, new MockEnvironment()); + ConfigurationCache.clear(); + } + + @AfterEach + void afterEach() throws Exception { + LockerManagerFactory.destroy(); + new FileLocker(null).cleanAllLocks(); + ConfigurationCache.clear(); + restoreEnvironment(); + } + + @Test + void testDefaultStoreSelectsFileLockerAndReleasesOwnerHolder() throws Exception { + FileLockManager lockManager = install(new DefaultFileLockStore()); + BranchSession owner = branchSession(1001L, 1L, "t_order:1"); + BranchSession contender = branchSession(1002L, 2L, "t_order:1"); + + Assertions.assertInstanceOf(FileLocker.class, lockManager.getLocker(owner)); + Assertions.assertTrue(lockManager.acquireLock(owner)); + Assertions.assertFalse(lockManager.acquireLock(contender)); + + owner.setLockKey(""); + Assertions.assertTrue(lockManager.releaseLock(owner)); + Assertions.assertTrue(owner.getLockHolder().isEmpty()); + Assertions.assertTrue(lockManager.acquireLock(contender)); + } + + @Test + void testDefaultStorePreservesGlobalReleaseAndLockerSemantics() throws Exception { + FileLockManager lockManager = install(new DefaultFileLockStore()); + GlobalSession globalSession = new GlobalSession("app", "group", "tx", 60000); + BranchSession first = branchSession(globalSession.getTransactionId(), 1L, "t_order:1"); + BranchSession second = branchSession(globalSession.getTransactionId(), 2L, "t_order:2"); + first.setXid(globalSession.getXid()); + second.setXid(globalSession.getXid()); + globalSession.add(first); + globalSession.add(second); + + Assertions.assertTrue(lockManager.acquireLock(first)); + Assertions.assertTrue(lockManager.acquireLock(second)); + Assertions.assertFalse(lockManager.isLockable(xid(2001L), first.getResourceId(), "t_order:1,2")); + + lockManager.updateLockStatus(globalSession.getXid(), LockStatus.Rollbacking); + Assertions.assertEquals(LockStatus.Locked, first.getLockStatus()); + + first.setLockKey(""); + second.setLockKey(""); + Assertions.assertTrue(lockManager.releaseGlobalSessionLock(globalSession)); + Assertions.assertTrue(first.getLockHolder().isEmpty()); + Assertions.assertTrue(second.getLockHolder().isEmpty()); + Assertions.assertTrue(lockManager.isLockable(xid(2001L), first.getResourceId(), "t_order:1,2")); + + BranchSession owner = branchSession(3001L, 3L, "t_order:3"); + Assertions.assertTrue(lockManager.acquireLock(owner)); + lockManager.cleanAllLocks(); + Assertions.assertTrue(lockManager.isLockable(xid(3002L), owner.getResourceId(), "t_order:3")); + } + + @Test + void testDefaultStoreReturnsFalseWhenOwnerReleaseFails() throws Exception { + FileLockManager lockManager = install(new DefaultFileLockStore()); + BranchSession failingOwner = failingOwner(); + + Assertions.assertFalse(lockManager.releaseLock(failingOwner)); + } + + @Test + void testDefaultGlobalReleaseContinuesAfterFailureAndReturnsLastSuccess() throws Exception { + FileLockManager lockManager = install(new DefaultFileLockStore()); + GlobalSession globalSession = new GlobalSession("app", "group", "tx", 60000); + BranchSession successfulOwner = branchSession(globalSession.getTransactionId(), 2L, "t_order:2"); + globalSession.add(failingOwner()); + globalSession.add(successfulOwner); + Assertions.assertTrue(lockManager.acquireLock(successfulOwner)); + + Assertions.assertTrue(lockManager.releaseGlobalSessionLock(globalSession)); Review Comment: This assertion codifies the partial-release bug: one branch has already failed, so returning success hides a leaked global lock. After aggregating all branch results, this expectation should be false (and the test name should describe all-branches success semantics). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
