IGNITE-27652 Refactor RingMessageWorker#sendMessageToClients #12663
Conversation
| * @throws IOException If serialization fails. | ||
| */ | ||
| byte[] serializeMessage(TcpDiscoveryAbstractMessage msg) throws IgniteCheckedException, IOException { | ||
| synchronized byte[] serializeMessage(TcpDiscoveryAbstractMessage msg) throws IgniteCheckedException, IOException { |
There was a problem hiding this comment.
Because there is a race condition between RingMessageWorker and ClientMessageWorker.
I have attached reproducer in the ticket, we can get assertion error during message write.
There was a problem hiding this comment.
maybe we should do something like:
synchronized(ses) {
ses.serializeMessage(msg);
}
and
synchronized(clientMsgWorker.ses) {
clientMsgWorker.ses.serializeMessage(msg)
}
where it is actually required. Server i/o doesn't need this synchronization due to its nature. At least for now.
There was a problem hiding this comment.
@Vladsz83 , on one hand you are right, but on the other hand modifier synchronized in method signature is a part of contract of class.
| private final Socket sock; | ||
|
|
||
| /** */ | ||
| /** Message writer. Access should be thread-safe. */ |
There was a problem hiding this comment.
Minor suggestion: Just '... Should be thread-safe.' Same below.
There was a problem hiding this comment.
Like this?
| /** Message writer. Access should be thread-safe. */ | |
| /** Message writer. Should be thread-safe. */ |
| * @throws IOException If serialization fails. | ||
| */ | ||
| byte[] serializeMessage(TcpDiscoveryAbstractMessage msg) throws IgniteCheckedException, IOException { | ||
| synchronized byte[] serializeMessage(TcpDiscoveryAbstractMessage msg) throws IgniteCheckedException, IOException { |
There was a problem hiding this comment.
maybe we should do something like:
synchronized(ses) {
ses.serializeMessage(msg);
}
and
synchronized(clientMsgWorker.ses) {
clientMsgWorker.ses.serializeMessage(msg)
}
where it is actually required. Server i/o doesn't need this synchronization due to its nature. At least for now.
shishkovilja
force-pushed
the
ignite-27652
branch
from
ad2c2e3 to
c9692f3
Compare
shishkovilja
changed the title
|
3 participants
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
The Contribution Checklist
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summarywhereXXXX- number of JIRA issue.(see the Maintainers list)
the
green visaattached to the JIRA ticket (see TC.Bot: Check PR)Notes
If you need any help, please email [email protected] or ask anу advice on http://asf.slack.com #ignite channel.