[ISSUE #4804] Fix SubStreamHandler exception loop by closeOnError #4807

Pil0tXia · 2024-04-02T12:18:22Z

Motivation

Please see #4804.

Modifications

Set the sender to null in the senderOnComplete() method, which requires the sender to be recreated in sendSubscription() after an error occurs. The only caller of the senderOnComplete() method is the close() method.
Add close() statement in onError() methods.

Existing problem: After calling the close() method, the SubStreamHandler instance will never run again unless the EventMeshGrpcConsumer re-subscribes, which is a known issue.

Documentation

Does this pull request introduce a new feature? (no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
If a feature is not applicable for documentation, explain why?
If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

codecov · 2024-04-02T12:35:21Z

Codecov Report

Attention: Patch coverage is 60.00000% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 16.35%. Comparing base (172804a) to head (f5898d8).

Files	Patch %	Lines
...sh/client/grpc/consumer/EventMeshGrpcConsumer.java	71.42%	2 Missing ⚠️
...entmesh/client/grpc/consumer/SubStreamHandler.java	33.33%	2 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             master    #4807       +/-   ##
=============================================
+ Coverage          0   16.35%   +16.35%     
- Complexity        0     1736     +1736     
=============================================
  Files             0      856      +856     
  Lines             0    31252    +31252     
  Branches          0     2698     +2698     
=============================================
+ Hits              0     5110     +5110     
- Misses            0    25665    +25665     
- Partials          0      477      +477

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mxsm

@Pil0tXia Code format used EventMesh code style xml?

Pil0tXia · 2024-04-03T12:03:46Z

@mxsm

The change in f5898d8 was made manually.

mxsm

LGMT

...tmesh-sdk-java/src/main/java/org/apache/eventmesh/client/grpc/consumer/SubStreamHandler.java

codecov-commenter · 2024-04-11T16:32:03Z

Codecov Report

Attention: Patch coverage is 60.00000% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 16.35%. Comparing base (172804a) to head (f5898d8).
Report is 8 commits behind head on master.

❗ Current head f5898d8 differs from pull request most recent head dbabca7. Consider uploading reports for the commit dbabca7 to get more accurate results

Files	Patch %	Lines
...sh/client/grpc/consumer/EventMeshGrpcConsumer.java	71.42%	2 Missing ⚠️
...entmesh/client/grpc/consumer/SubStreamHandler.java	33.33%	2 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             master    #4807       +/-   ##
=============================================
+ Coverage          0   16.35%   +16.35%     
- Complexity        0     1736     +1736     
=============================================
  Files             0      856      +856     
  Lines             0    31252    +31252     
  Branches          0     2698     +2698     
=============================================
+ Hits              0     5110     +5110     
- Misses            0    25665    +25665     
- Partials          0      477      +477

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

...tmesh-sdk-java/src/main/java/org/apache/eventmesh/client/grpc/consumer/SubStreamHandler.java

This reverts commit 774397f.

...tmesh-sdk-java/src/main/java/org/apache/eventmesh/client/grpc/consumer/SubStreamHandler.java

pandaapo · 2024-04-13T10:15:50Z

...tmesh-sdk-java/src/main/java/org/apache/eventmesh/client/grpc/consumer/SubStreamHandler.java

-            senderOnComplete();
+        synchronized (sender) {
+            if (isClosed) {
+                return;


Firstly, setting aside the context, when this flag variable is wrapped by synchronized, its volatile is redundant and even adds overhead.

Returning to the context, as explained earlier, there's no need to add another synchronized outside of senderOnComplete()'s lock. If use the approach previously mentioned of adding a flag, consider using AtomicBoolean.

I think a synchronized block is necessary to keep isClosed flag judgment and operation atomic. Even with AtomicBoolean, atomicity is only guaranteed for a single operation. Without the synchronized code block, it is possible for the second thread to pass the isClosed == false judgment before isClosed is set to true.

I think a synchronized block is necessary to keep isClosed flag judgment and operation atomic. Even with AtomicBoolean, atomicity is only guaranteed for a single operation. Without the synchronized code block, it is possible for the second thread to pass the isClosed == false judgment before isClosed is set to true.

You can learn about some methods of AtomicBoolean.

* [ISSUE #4812] Set up Admin Endpoints v2 (#4813) * Remove redundant overloaded methods * Simplify write() result param * Add writeJson(); Add PUT; Add JavaDoc * Rename EventHttpHandler to EventMeshHttpHandler * Correct server thread name * Clean up messy & non-hierarchical overloading * No need to set headers manually any more * Set up v1&v2 endpoints * Set up v1&v2 response dto * Introduce fastjson2 * Fix fastjson2 "level too large : 2048" error caused by IPAddress * Correct @ConfigField naming * Return properties format json key * Add format option to query string * Introduce Result * Reduce duplicate builder code * Fix all checkstyle warnings in eventmesh-runtime * Add known dependency * [ISSUE #4814] Migrate from fastjson 1.2.83 to fastjson2 (#4819) * [Enhancement] Migrate from fastjson 1.2.83 to fastjson2 #4814 * fix_dependencies_problem * fix_check * [ISSUE #4551] modify the logic of time-consumption statistics (#4822) * init connector runtime v2 * [ISSUE #4804] Fix SubStreamHandler exception loop by closeOnError (#4807) * Handle exception loop by closeOnError * Lombok optimization * some format optimization * Avoid closing multiple times * Remove redundant set null * Revert "Avoid closing multiple times" This reverts commit 774397f. * Use synchronized latch to keep senderOnComplete called once * Use boolean to prevent latch called by somebody else * Remove the unique callee/caller close() of onCompleted() * [ISSUE #4838] Deprecate unused `eventMesh.connector.plugin.type` etc. properties (#4839) * Remove all references of `eventMesh.connector.plugin.type` * Deprecate `eventMesh.connector.plugin.type` and sort properties * Remove misconfigured & not-used `registerIntervalInMills`, `fetchRegistryAddrIntervalInMills` * Remove 'defibus' related un-used usages * Supplement #4809 for `null != object` * [ISSUE #4832] Downgrade stale bot to v8 to resolve state cache reserving error (#4833) * Revert stale bot to v8 to resolve state cache reserving error * Reduce operations-per-run to default value to ease pressure * Unify yaml to yml * [ISSUE #4820] Bug fix EventHandler not return json (#4821) * bug fix * bug fix * bug fix * update runtime v2 * update connector runtime * update connector runtime * update connector runtime * update connector runtime * update connector runtime --------- Co-authored-by: Pil0tXia <[email protected]> Co-authored-by: Zaki <[email protected]> Co-authored-by: Karson <[email protected]>

* EventMesh function admin (#4851) * own * dependency * finish registry * EventMesh function admin (#4853) * own * dependency * finish registry * init * Eventmesh function admin (#4854) * own * dependency * finish registry * init * 0419 * 0419 * more discovery and move gRPC * fix dependency * EventMesh function connector runtime (#4858) * [ISSUE #4812] Set up Admin Endpoints v2 (#4813) * Remove redundant overloaded methods * Simplify write() result param * Add writeJson(); Add PUT; Add JavaDoc * Rename EventHttpHandler to EventMeshHttpHandler * Correct server thread name * Clean up messy & non-hierarchical overloading * No need to set headers manually any more * Set up v1&v2 endpoints * Set up v1&v2 response dto * Introduce fastjson2 * Fix fastjson2 "level too large : 2048" error caused by IPAddress * Correct @ConfigField naming * Return properties format json key * Add format option to query string * Introduce Result * Reduce duplicate builder code * Fix all checkstyle warnings in eventmesh-runtime * Add known dependency * [ISSUE #4814] Migrate from fastjson 1.2.83 to fastjson2 (#4819) * [Enhancement] Migrate from fastjson 1.2.83 to fastjson2 #4814 * fix_dependencies_problem * fix_check * [ISSUE #4551] modify the logic of time-consumption statistics (#4822) * init connector runtime v2 * [ISSUE #4804] Fix SubStreamHandler exception loop by closeOnError (#4807) * Handle exception loop by closeOnError * Lombok optimization * some format optimization * Avoid closing multiple times * Remove redundant set null * Revert "Avoid closing multiple times" This reverts commit 774397f. * Use synchronized latch to keep senderOnComplete called once * Use boolean to prevent latch called by somebody else * Remove the unique callee/caller close() of onCompleted() * [ISSUE #4838] Deprecate unused `eventMesh.connector.plugin.type` etc. properties (#4839) * Remove all references of `eventMesh.connector.plugin.type` * Deprecate `eventMesh.connector.plugin.type` and sort properties * Remove misconfigured & not-used `registerIntervalInMills`, `fetchRegistryAddrIntervalInMills` * Remove 'defibus' related un-used usages * Supplement #4809 for `null != object` * [ISSUE #4832] Downgrade stale bot to v8 to resolve state cache reserving error (#4833) * Revert stale bot to v8 to resolve state cache reserving error * Reduce operations-per-run to default value to ease pressure * Unify yaml to yml * [ISSUE #4820] Bug fix EventHandler not return json (#4821) * bug fix * bug fix * bug fix * update runtime v2 * update connector runtime * update connector runtime * update connector runtime * update connector runtime * update connector runtime --------- Co-authored-by: Pil0tXia <[email protected]> Co-authored-by: Zaki <[email protected]> Co-authored-by: Karson <[email protected]> * [ISSUE #4931]Add Registry Module for Discovery AdminServer * [ISSUES #4933]Add Admin Module * [ISSUE #4935] Add and Move the Pojo Used By Both Runtime and Admin to Common * [ISSUE #4937]fix gradle dependecy and add runtime v2 * [ISSUES #4939]add canal connector * fix missing apache header * fix missing apache header * fix missing apache header * update gradle dependencies * fix admin server ci check error * fix admin server ci check error * fix ci checkStyle error * fix ci check error --------- Co-authored-by: sodaRyCN <[email protected]> Co-authored-by: Pil0tXia <[email protected]> Co-authored-by: Zaki <[email protected]> Co-authored-by: Karson <[email protected]>

* EventMesh function admin (#4851) * own * dependency * finish registry * EventMesh function admin (#4853) * own * dependency * finish registry * init * Eventmesh function admin (#4854) * own * dependency * finish registry * init * 0419 * 0419 * more discovery and move gRPC * fix dependency * EventMesh function connector runtime (#4858) * [ISSUE #4812] Set up Admin Endpoints v2 (#4813) * Remove redundant overloaded methods * Simplify write() result param * Add writeJson(); Add PUT; Add JavaDoc * Rename EventHttpHandler to EventMeshHttpHandler * Correct server thread name * Clean up messy & non-hierarchical overloading * No need to set headers manually any more * Set up v1&v2 endpoints * Set up v1&v2 response dto * Introduce fastjson2 * Fix fastjson2 "level too large : 2048" error caused by IPAddress * Correct @ConfigField naming * Return properties format json key * Add format option to query string * Introduce Result * Reduce duplicate builder code * Fix all checkstyle warnings in eventmesh-runtime * Add known dependency * [ISSUE #4814] Migrate from fastjson 1.2.83 to fastjson2 (#4819) * [Enhancement] Migrate from fastjson 1.2.83 to fastjson2 #4814 * fix_dependencies_problem * fix_check * [ISSUE #4551] modify the logic of time-consumption statistics (#4822) * init connector runtime v2 * [ISSUE #4804] Fix SubStreamHandler exception loop by closeOnError (#4807) * Handle exception loop by closeOnError * Lombok optimization * some format optimization * Avoid closing multiple times * Remove redundant set null * Revert "Avoid closing multiple times" This reverts commit 774397f. * Use synchronized latch to keep senderOnComplete called once * Use boolean to prevent latch called by somebody else * Remove the unique callee/caller close() of onCompleted() * [ISSUE #4838] Deprecate unused `eventMesh.connector.plugin.type` etc. properties (#4839) * Remove all references of `eventMesh.connector.plugin.type` * Deprecate `eventMesh.connector.plugin.type` and sort properties * Remove misconfigured & not-used `registerIntervalInMills`, `fetchRegistryAddrIntervalInMills` * Remove 'defibus' related un-used usages * Supplement #4809 for `null != object` * [ISSUE #4832] Downgrade stale bot to v8 to resolve state cache reserving error (#4833) * Revert stale bot to v8 to resolve state cache reserving error * Reduce operations-per-run to default value to ease pressure * Unify yaml to yml * [ISSUE #4820] Bug fix EventHandler not return json (#4821) * bug fix * bug fix * bug fix * update runtime v2 * update connector runtime * update connector runtime * update connector runtime * update connector runtime * update connector runtime --------- Co-authored-by: Pil0tXia <[email protected]> Co-authored-by: Zaki <[email protected]> Co-authored-by: Karson <[email protected]> * [ISSUE #4931]Add Registry Module for Discovery AdminServer * [ISSUES #4933]Add Admin Module * [ISSUE #4935] Add and Move the Pojo Used By Both Runtime and Admin to Common * [ISSUE #4937]fix gradle dependecy and add runtime v2 * [ISSUES #4939]add canal connector * fix missing apache header * fix missing apache header * fix missing apache header * update gradle dependencies * fix admin server ci check error * fix admin server ci check error * fix ci checkStyle error * fix ci check error * [ISSUE #4979]Canal Connector supports bidirectional data synchronization * add bash files for admin & runtime-v2 * fix ack offset read & persist * fix checkStyle error * [ISSUE #4979] Canal Connector supports bidirectional data synchronization (#5011) * [ISSUE #4979]Canal Connector supports bidirectional data synchronization * add bash files for admin & runtime-v2 * fix ack offset read & persist * fix checkStyle error * fix http source connector test error --------- Co-authored-by: sodaRyCN <[email protected]> Co-authored-by: Pil0tXia <[email protected]> Co-authored-by: Zaki <[email protected]> Co-authored-by: Karson <[email protected]>

Pil0tXia added 3 commits April 2, 2024 19:21

Handle exception loop by closeOnError

a540e8a

Lombok optimization

e9c7ce9

some format optimization

f5898d8

Pil0tXia mentioned this pull request Apr 3, 2024

[Bug] grpc SubStreamHandler exception #4804

Closed

3 tasks

mxsm reviewed Apr 3, 2024

View reviewed changes

Pil0tXia requested a review from mxsm April 8, 2024 13:28

mxsm previously approved these changes Apr 9, 2024

View reviewed changes

pandaapo reviewed Apr 10, 2024

View reviewed changes

...tmesh-sdk-java/src/main/java/org/apache/eventmesh/client/grpc/consumer/SubStreamHandler.java Outdated Show resolved Hide resolved

Pil0tXia dismissed mxsm’s stale review via 774397f April 11, 2024 16:30

Avoid closing multiple times

774397f

Pil0tXia requested review from mxsm and pandaapo April 11, 2024 16:31

pandaapo reviewed Apr 12, 2024

View reviewed changes

...tmesh-sdk-java/src/main/java/org/apache/eventmesh/client/grpc/consumer/SubStreamHandler.java Outdated Show resolved Hide resolved

...tmesh-sdk-java/src/main/java/org/apache/eventmesh/client/grpc/consumer/SubStreamHandler.java Outdated Show resolved Hide resolved

Pil0tXia added 3 commits April 12, 2024 14:09

Remove redundant set null

d0ac6c0

Revert "Avoid closing multiple times"

878009a

This reverts commit 774397f.

Use synchronized latch to keep senderOnComplete called once

8e812cd

pandaapo reviewed Apr 13, 2024

View reviewed changes

...tmesh-sdk-java/src/main/java/org/apache/eventmesh/client/grpc/consumer/SubStreamHandler.java Outdated Show resolved Hide resolved

Use boolean to prevent latch called by somebody else

4d86948

pandaapo reviewed Apr 13, 2024

View reviewed changes

Remove the unique callee/caller close() of onCompleted()

dbabca7

pandaapo approved these changes Apr 13, 2024

View reviewed changes

mxsm approved these changes Apr 13, 2024

View reviewed changes

mxsm merged commit fe2bd7c into apache:master Apr 13, 2024
9 checks passed

Pil0tXia deleted the pil0txia/bug_4804 branch April 13, 2024 14:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ISSUE #4804] Fix SubStreamHandler exception loop by closeOnError #4807

[ISSUE #4804] Fix SubStreamHandler exception loop by closeOnError #4807

Pil0tXia commented Apr 2, 2024

codecov bot commented Apr 2, 2024 •

edited

Loading

mxsm left a comment

Pil0tXia commented Apr 3, 2024

mxsm left a comment

codecov-commenter commented Apr 11, 2024 •

edited

Loading

pandaapo Apr 13, 2024

Pil0tXia Apr 13, 2024

pandaapo Apr 13, 2024

[ISSUE #4804] Fix SubStreamHandler exception loop by closeOnError #4807

[ISSUE #4804] Fix SubStreamHandler exception loop by closeOnError #4807

Conversation

Pil0tXia commented Apr 2, 2024

Motivation

Modifications

Documentation

codecov bot commented Apr 2, 2024 • edited Loading

Codecov Report

mxsm left a comment

Choose a reason for hiding this comment

Pil0tXia commented Apr 3, 2024

mxsm left a comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 11, 2024 • edited Loading

Codecov Report

pandaapo Apr 13, 2024

Choose a reason for hiding this comment

Pil0tXia Apr 13, 2024

Choose a reason for hiding this comment

pandaapo Apr 13, 2024

Choose a reason for hiding this comment

codecov bot commented Apr 2, 2024 •

edited

Loading

codecov-commenter commented Apr 11, 2024 •

edited

Loading