bugfix: Resolve NullPointer and port binding errors#7397
bugfix: Resolve NullPointer and port binding errors#7397funky-eyes merged 4 commits intoapache:2.xfrom
Conversation
Signed-off-by: yongjunhong <yongjunh@apache.org>
Signed-off-by: yongjunhong <yongjunh@apache.org>
|
Error: Failures: |
|
This issue has occurred since #7363, but it seems that this PR has not updated the content associated with UT.So, this has been a potential issue, some execution environment triggers this issue? |
I’m not exactly sure which execution environment is triggering this issue. The first line of defense against such potential issues would be during code review — by carefully checking for things like possible NPEs. But of course, this level of thorough review can be resource-intensive and difficult to maintain consistently. Secondly, I think it’s important to conduct a more detailed investigation into why this kind of issue is happening intermittently. This likely requires effort and resources from the PMC and Committers to properly resolve. Do you happen to have any ideas or suspicions about what might be causing this? |
Signed-off-by: yongjunhong <yongjunh@apache.org>
YongGoose
force-pushed
the
bugfix-20250531
branch
from
c775ac4 to
c3bb76e
Compare
| globalSession.changeGlobalStatus(GlobalStatus.CommitFailed); | ||
| SingleResult<Void> singleResult = globalSessionService.changeGlobalStatus(xid); | ||
|
|
||
| Assertions.assertTrue(singleResult.isSuccess()); | ||
| Assertions.assertEquals(GlobalStatus.Committed, globalSession.getStatus()); |
There was a problem hiding this comment.
I’ve made some changes to the test code — would you mind taking a look and giving some feedback?
Specifically, I’m wondering whether it’s correct for changeGlobalStatus to set the status to Committed when the globalSession is initially set to CommitFailed.
When I run the tests locally, the status remains CommitFailed, but it seems status is changed to Committed in the CI environment.
A new issue has come up. 🤯
Error: Errors:
Error: FileSessionManagerTest.restartBranchFailRetryTest:629 » NullPointer Cannot inv...
Error: FileTransactionStoreManagerTest.init:55 » NullPointer Cannot invoke "Object.ha...
There was a problem hiding this comment.
The failing unit tests run successfully locally, but encounter failures in the CI environment. The primary cause of failures is concurrency conflicts during test execution. After analyzing recent failed and successful CI test runs, a common factor among the failed tests is that the failing modules and the seata-namingserver module were executed in parallel. In successful CI pipelines, the execution order of seata-namingserver differs compared to failed pipelines.
Failed CI Test Runs:
Building seata-namingserver 2.5.0-SNAPSHOT 2.5.0-SNAPSHOT [46/95]
Successful CI Test Runs:
Building seata-namingserver 2.5.0-SNAPSHOT 2.5.0-SNAPSHOT [80/95]
There was a problem hiding this comment.
This issue is currently blocking the release of version 2.4.0. As a short-term mitigation, we can temporarily resolve it by controlling the execution order of unit tests to avoid concurrency conflicts. In the long term, we need to implement robust support for concurrent multi-module testing.
There was a problem hiding this comment.
Tomorrow is a public holiday in South Korea, so I’ll make sure to finish the work by then.
I’ll also try reordering the unit tests as you suggested.
There was a problem hiding this comment.
This issue is currently blocking the release of version 2.4.0. As a short-term mitigation, we can temporarily resolve it by controlling the execution order of unit tests to avoid concurrency conflicts. In the long term, we need to implement robust support for concurrent multi-module testing.
Would it be better to create a separate issue for this?
If you have any suggestions or ideas on how to approach it, I’d really appreciate your comments!
|
Should we have the system randomly allocate a free port instead of manually specifying it for port binding issues? |
|
I noticed that Seata encountered an error while attempting to restore a BranchSession from a file — the transactionId was 0, which is invalid and caused the restore to fail. |
|
I think the issue is caused by changes in the CI running environment. Perhaps we should specify a stable Linux environment and JDK version (rather than using the latest version). |
Feel free to update my PR to test that case — the Allow edits and access to secrets by maintainers setting is enabled. |
|
I’m unable to access the link you shared — it shows an AuthenticationFailed error. |
Signed-off-by: yongjunhong <yongjunh@apache.org>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 2.x #7397 +/- ##
============================================
+ Coverage 58.44% 58.77% +0.32%
Complexity 535 535
============================================
Files 1272 1272
Lines 45862 45862
Branches 5522 5522
============================================
+ Hits 26806 26957 +151
+ Misses 16532 16351 -181
- Partials 2524 2554 +30 see 54 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
This change has increased the CI execution time by about one minute.
|
There was a problem hiding this comment.
Pull Request Overview
This PR addresses NullPointerException and port binding errors, improves test setup/teardown to avoid conflicts, and updates changelogs for the 2.x branch.
- Added changelog entries in English and Chinese for bug #7397.
- Switched server port in an integration test and enhanced unit tests with proper lifecycle hooks.
- Replaced static
@BeforeAll/@AfterAllwith@BeforeEach/@AfterEachin session store tests and added mocks in channel listener tests.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/test/java/.../FileTransactionStoreManagerTest.java | Swapped @BeforeAll/@AfterAll for per-test setup/teardown. |
| server/src/test/java/.../FileSessionManagerTest.java | Replaced class-level hooks with @BeforeEach/@AfterEach, removed redundant SessionHolder.init calls, and simplified a test. |
| core/src/test/java/.../ChannelEventListenerTest.java | Added ChannelId mock and when(channel.id()) stubbing. |
| core/src/test/java/.../ChannelEventHandlerIntegrationTest.java | Updated the hardcoded server port from 8091 to 8919. |
| changes/zh-cn/2.x.md | Inserted bugfix entry and adjusted spacing in the Chinese changelog. |
| changes/en-us/2.x.md | Inserted bugfix entry in the English changelog. |
Comments suppressed due to low confidence (1)
server/src/test/java/org/apache/seata/server/session/FileSessionManagerTest.java:472
- This TODO indicates missing test coverage for global session failure status transitions; consider adding tests to verify FAIL_COMMIT and FAIL_ROLLBACK states as noted.
// TODO: After implementing robust support for concurrent multi-module tests, add tests to verify that globalSession transitions to FAIL_COMMIT_STATUS and FAIL_ROLLBACK_STATUS.
The current order is still in the wrong order, but it looks like the resource cleanup in PR has solved the problem. You can follow these steps to check:
|
Thank you so much for your detailed response! 👍🏻 The change is only a temporary, so we’ll need to take further steps to ensure long-term test stability. Thanks again! |
4 participants
This pull request includes several updates to improve test reliability, fix bugs, and update documentation for the
2.xbranch. Key changes include resolving NullPointer and port binding errors, improving test setup and teardown processes, and adding missing mock configurations.Bug Fixes:
2.x.mdchangelogs to document the resolution of NullPointer and port binding errors (#7397). [1] [2]Test Improvements:
ChannelEventHandlerIntegrationTestto use a different server port (8919) to avoid potential conflicts.ChannelEventListenerTestby adding a mock forChannelIdand usingwhento simulatechannel.id()behavior. [1] [2] [3] [4] [5]FileSessionManagerTestby introducing@BeforeEachand@AfterEachmethods to initialize and clean upSessionHolder, ensuring consistent test states. Removed redundantSessionHolder.initcalls within individual tests. [1] [2] [3] [4] [5] [6] [7] [8] [9]FileTransactionStoreManagerTestto use@BeforeEachand@AfterEachforSessionHoldersetup and teardown, replacing outdated@BeforeAlland@AfterAllmethods. [1] [2]Documentation Updates:
2.x.mdchangelog under the "optimize" and "refactor" sections for better formatting. [1] [2]