Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfix: eliminate the impact of instructions reordering on session async committing task #1576

Merged
merged 3 commits into from Sep 12, 2019

Conversation

ggndnn
Copy link
Contributor

@ggndnn ggndnn commented Sep 2, 2019

Ⅰ. Describe what this PR did

After code analysis, I found there may be instructions reordering in DefaultCore#asyncCommit. Session object may be added to async committing session manager first, before globalSession.addSessionLifecycleListener and globalSession.changeStatus. The reason is that GlobalSession#lifecycleListeners and DefaultSessionManager#sessionMap have no dependency to each other.

    private void asyncCommit(GlobalSession globalSession) throws TransactionException {
        globalSession.addSessionLifecycleListener(SessionHolder.getAsyncCommittingSessionManager());
        SessionHolder.getAsyncCommittingSessionManager().addGlobalSession(globalSession);
        globalSession.changeStatus(GlobalStatus.AsyncCommitting);
    }

I think, unexpected scheduling delay of async committing job and instructions reordering lead to the situation happened in #1564.

The analysis of one error log:

[INFO] Running io.seata.server.event.DefaultCoreForEventBusTest
2019-08-30 09:34:45.530 INFO [UndoLogDelete_1]io.seata.server.coordinator.DefaultCoordinator.undoLogDelete:403 -no active rm channels to delete undo log
2019-08-30 09:34:45.629 INFO [AsyncCommitting_1]io.seata.server.coordinator.DefaultCore.doGlobalCommit:238 -Global[172.17.0.1:0:4020856891] committing is successfully done.
2019-08-30 09:34:45.630 INFO [AsyncCommitting_1]io.seata.server.coordinator.DefaultCore.doGlobalCommit:238 -Global[172.17.0.1:0:4020856891] committing is successfully done.
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 1.035 s <<< FAILURE! - in io.seata.server.event.DefaultCoreForEventBusTest
[ERROR] test  Time elapsed: 1.034 s  <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <1> but was: <2>
	at io.seata.server.event.DefaultCoreForEventBusTest.test(DefaultCoreForEventBusTest.java:82)
  • First run of async committing job delayed to 09:34:45.629, session status was not changed to AsyncCommitting until after DefaultCore#doGlobalCommit#endCommitted, session object still be in session manager. One AsyncCommitting event was posted here.
  • Second run of async committing job started as scheduled at 09:34:45.630, another AsyncCommitting event was posted then.

This PR only add a session status check, any better idea?

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@codecov-io
Copy link

codecov-io commented Sep 2, 2019

Codecov Report

Merging #1576 into develop will increase coverage by 0.17%.
The diff coverage is 0%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #1576      +/-   ##
=============================================
+ Coverage      47.08%   47.25%   +0.17%     
  Complexity      1764     1764              
=============================================
  Files            359      359              
  Lines          12971    12999      +28     
  Branches        1579     1617      +38     
=============================================
+ Hits            6107     6143      +36     
- Misses          6187     6191       +4     
+ Partials         677      665      -12
Impacted Files Coverage Δ Complexity Δ
...o/seata/server/coordinator/DefaultCoordinator.java 50% <0%> (-0.46%) 26 <0> (ø)
...in/java/io/seata/server/session/GlobalSession.java 84.54% <0%> (-0.61%) 67% <0%> (ø)
...a/io/seata/core/rpc/netty/AbstractRpcRemoting.java 9.95% <0%> (-0.21%) 3% <0%> (ø)
...n/java/io/seata/rm/datasource/ConnectionProxy.java 22.88% <0%> (-0.2%) 10% <0%> (ø)
...tobuf/convertor/BranchCommitResponseConvertor.java 97.43% <0%> (+0.06%) 3% <0%> (ø) ⬇️
...in/java/io/seata/server/session/BranchSession.java 78.19% <0%> (+0.07%) 41% <0%> (ø) ⬇️
...buf/convertor/BranchRollbackResponseConvertor.java 97.36% <0%> (+0.07%) 3% <0%> (ø) ⬇️
...tobuf/convertor/GlobalCommitResponseConvertor.java 97.05% <0%> (+0.08%) 3% <0%> (ø) ⬇️
...buf/convertor/GlobalRollbackResponseConvertor.java 97.05% <0%> (+0.08%) 3% <0%> (ø) ⬇️
...tobuf/convertor/GlobalStatusResponseConvertor.java 97.05% <0%> (+0.08%) 3% <0%> (ø) ⬇️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9877828...4f1ead9. Read the comment docs.

@ggndnn ggndnn changed the title bugfix: prevent a session async committing job from being scheduled incorrectly bugfix: eliminate the impact of instructions reordering on session async committing task Sep 3, 2019
Copy link
Member

@xingfudeshi xingfudeshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Contributor

@zjinlei zjinlei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants