[ISSUE #4695] Fix tcp client not thread safe #4696
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4696 +/- ##
=========================================
Coverage 17.39% 17.39%
Complexity 1759 1759
=========================================
Files 797 797
Lines 29850 29850
Branches 2579 2579
=========================================
Hits 5192 5192
Misses 24177 24177
Partials 481 481 ☔ View full report in Codecov by Sentry. |
| contexts.put(key, context); | ||
| } else { | ||
| LogUtils.info(log, "duplicate key : {}", key); | ||
| synchronized (context) { |
There was a problem hiding this comment.
Using context as a lock object does not seem to achieve thread safety, as context will be different objects in concurrent situations.
Perhaps ConcurrentHashMap#putIfAbsent() is one of the optional solutions.
There was a problem hiding this comment.
Really exist the above-mentioned problem , It has been recommitted for review. Thank you.
| synchronized (context) { | ||
| if (!contexts.containsValue(context)) { | ||
| contexts.put(key, context); | ||
| } else { | ||
| LogUtils.info(log, "duplicate key : {}", key); | ||
| } |
There was a problem hiding this comment.
When the client sends two identical messages due to network issues, the existing logic can avoid duplicate keys. However, if the client sends two messages with the same content but different seq due to business issues, the existing logic, when running in a thread-unsafe manner, will retain the key and message from the first received package and will not be able to output a duplicate key log. Therefore, it is still necessary to use putIfAbsent.
| synchronized (context) { | |
| if (!contexts.containsValue(context)) { | |
| contexts.put(key, context); | |
| } else { | |
| LogUtils.info(log, "duplicate key : {}", key); | |
| } | |
| RequestContext previousContext = contexts.putIfAbsent(key, context); | |
| if (previousContext != null) { | |
| LogUtils.info(log, "duplicate key : {}", key); | |
| } |
There was a problem hiding this comment.
Please review again, thanks
|
|
||
| package org.apache.eventmesh.client.tcp.common; | ||
|
|
||
| import java.util.Objects; |
There was a problem hiding this comment.
Please remove redundant import for successful CI.
| import org.apache.eventmesh.common.protocol.tcp.Package; | ||
| import org.apache.eventmesh.common.protocol.tcp.UserAgent; | ||
| import org.apache.eventmesh.common.protocol.tcp.codec.Codec; | ||
| import org.apache.eventmesh.common.utils.LogUtils; |
There was a problem hiding this comment.
CI will report imports order error after this modification. You can refer to https://eventmesh.apache.org/community/contribute/contribute#code-style to help check checkstyle.
* fix tcp client thread not safe * fix bug * fix bug * fix bug * fix bug * fix bug * fix bug * fix style problem * fix style problem * fix style problem
4 participants
Fixes #4695.
Motivation
Explain the content here.
Explain why you want to make the changes and what problem you're trying to solve.
Modifications
Describe the modifications you've done.
Documentation