[fix][broker] Copy proto command fields into final variables in ServerCnx #18987

michaeljmarshall · 2022-12-19T18:57:27Z

Motivation

In the PulsarDecoder, we use a single BaseCommand object and overwrite it for each incoming protocol message. As a result, it is not safe to publish any references to a proto command to other threads.

Here is the single BaseCommand:

pulsar/pulsar-common/src/main/java/org/apache/pulsar/common/protocol/PulsarDecoder.java

Line 99 in 82237d3

private final BaseCommand cmd = new BaseCommand();

Here is the method call that resets the object:

pulsar/pulsar-common/src/main/java/org/apache/pulsar/common/protocol/PulsarDecoder.java

Line 114 in 82237d3

cmd.parseFrom(buffer, cmdSize);

Note that the call to parseFrom first calls clear(), which resets all values on the object.

This PR copies relevant values or objects into other variables.

Modifications

Replace command with tcId since the latter is a final variable meant to be published to another thread.
Move logic to copy certain command fields to earlier in method for handleSubscribe
Copy ack object to new CommandAck when there is a broker interceptor. Note that copying this command is likely somewhat costly, so we only do it when there is an interceptor configured.

Verifying this change

This is a trivial change that is already covered by tests.

Documentation

doc-not-needed

This is an internal change.

Matching PR in forked repository

PR in forked repository: michaeljmarshall#8

… race

codecov-commenter · 2022-12-19T20:14:13Z

Codecov Report

Merging #18987 (886796a) into master (feb3cb4) will increase coverage by 0.06%.
The diff coverage is 71.42%.

@@             Coverage Diff              @@
##             master   #18987      +/-   ##
============================================
+ Coverage     46.35%   46.41%   +0.06%     
- Complexity     8939    10461    +1522     
============================================
  Files           597      703     +106     
  Lines         56858    68866   +12008     
  Branches       5905     7389    +1484     
============================================
+ Hits          26357    31967    +5610     
- Misses        27616    33292    +5676     
- Partials       2885     3607     +722

Flag	Coverage Δ
unittests	`46.41% <71.42%> (+0.06%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...va/org/apache/pulsar/broker/service/ServerCnx.java	`47.48% <66.66%> (-1.49%)`	⬇️
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java	`53.81% <100.00%> (ø)`
...lsar/broker/loadbalance/impl/ThresholdShedder.java	`3.27% <0.00%> (-27.05%)`	⬇️
...ar/broker/loadbalance/impl/BundleSplitterTask.java	`60.00% <0.00%> (-20.00%)`	⬇️
...roker/loadbalance/impl/ModularLoadManagerImpl.java	`58.72% <0.00%> (-9.57%)`	⬇️
...org/apache/pulsar/broker/loadbalance/LoadData.java	`58.33% <0.00%> (-8.34%)`	⬇️
.../pulsar/broker/service/BrokerServiceException.java	`38.88% <0.00%> (-7.41%)`	⬇️
...er/loadbalance/impl/ModularLoadManagerWrapper.java	`78.04% <0.00%> (-7.32%)`	⬇️
...g/apache/pulsar/client/impl/ConnectionHandler.java	`50.00% <0.00%> (-5.32%)`	⬇️
...rg/apache/pulsar/broker/service/AbstractTopic.java	`61.61% <0.00%> (-3.60%)`	⬇️
... and 151 more

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

lhotari · 2022-12-20T09:51:53Z

I added #18997 and #18998 as follow ups for this PR. Please review

michaeljmarshall · 2022-12-20T19:01:13Z

When attempting to cherry-pick this to branch-2.11, I ran into conflicts with #18193. Looking to see if we need to cherry pick that one first.

…rCnx (#18987) In the `PulsarDecoder`, we use a single `BaseCommand` object and overwrite it for each incoming protocol message. As a result, it is not safe to publish any references to a proto command to other threads. Here is the single `BaseCommand`: https://github.com/apache/pulsar/blob/82237d3684fe506bcb6426b3b23f413422e6e4fb/pulsar-common/src/main/java/org/apache/pulsar/common/protocol/PulsarDecoder.java#L99 Here is the method call that resets the object: https://github.com/apache/pulsar/blob/82237d3684fe506bcb6426b3b23f413422e6e4fb/pulsar-common/src/main/java/org/apache/pulsar/common/protocol/PulsarDecoder.java#L114 Note that the call to `parseFrom` first calls `clear()`, which resets all values on the object. This PR copies relevant values or objects into other variables. * Replace `command` with `tcId` since the latter is a final variable meant to be published to another thread. * Move logic to copy certain command fields to earlier in method for `handleSubscribe` * Copy `ack` object to new `CommandAck` when there is a broker interceptor. Note that copying this command is likely somewhat costly, so we only do it when there is an interceptor configured. This is a trivial change that is already covered by tests. - [x] `doc-not-needed` This is an internal change. PR in forked repository: michaeljmarshall#8 (cherry picked from commit a408e9e)

…rCnx (apache#18987) In the `PulsarDecoder`, we use a single `BaseCommand` object and overwrite it for each incoming protocol message. As a result, it is not safe to publish any references to a proto command to other threads. Here is the single `BaseCommand`: https://github.com/apache/pulsar/blob/82237d3684fe506bcb6426b3b23f413422e6e4fb/pulsar-common/src/main/java/org/apache/pulsar/common/protocol/PulsarDecoder.java#L99 Here is the method call that resets the object: https://github.com/apache/pulsar/blob/82237d3684fe506bcb6426b3b23f413422e6e4fb/pulsar-common/src/main/java/org/apache/pulsar/common/protocol/PulsarDecoder.java#L114 Note that the call to `parseFrom` first calls `clear()`, which resets all values on the object. This PR copies relevant values or objects into other variables. * Replace `command` with `tcId` since the latter is a final variable meant to be published to another thread. * Move logic to copy certain command fields to earlier in method for `handleSubscribe` * Copy `ack` object to new `CommandAck` when there is a broker interceptor. Note that copying this command is likely somewhat costly, so we only do it when there is an interceptor configured. This is a trivial change that is already covered by tests. - [x] `doc-not-needed` This is an internal change. PR in forked repository: michaeljmarshall#8 (cherry picked from commit a408e9e)

[fix][txn] Use correct tcId variable to prevent data race

e99cab9

michaeljmarshall added type/bug The PR fixed a bug or issue reported a bug area/broker area/transaction doc-not-needed Your PR changes do not impact docs labels Dec 19, 2022

michaeljmarshall added this to the 2.12.0 milestone Dec 19, 2022

michaeljmarshall requested review from lhotari, eolivelli, nicoloboschi and congbobo184 December 19, 2022 18:57

michaeljmarshall self-assigned this Dec 19, 2022

nicoloboschi approved these changes Dec 19, 2022

View reviewed changes

nicoloboschi added the ready-to-test label Dec 19, 2022

[fix][broker] ServerCnx copy subscribe command fields to prevent data…

9b2f363

… race

michaeljmarshall changed the title ~~[fix][txn] Use correct tcId variable to prevent data race~~ [fix][Broker] Copy proto command fields into final variables in ServerCnx Dec 19, 2022

michaeljmarshall changed the title ~~[fix][Broker] Copy proto command fields into final variables in ServerCnx~~ [fix][broker] Copy proto command fields into final variables in ServerCnx Dec 19, 2022

michaeljmarshall added 3 commits December 19, 2022 13:28

Cleanup extra new line

209de63

[fix][broker] Do not publish mutable ack to callback

9f8d94d

Code cleanup

886796a

codelipenghui approved these changes Dec 20, 2022

View reviewed changes

mattisonchao approved these changes Dec 20, 2022

View reviewed changes

nodece approved these changes Dec 20, 2022

View reviewed changes

michaeljmarshall merged commit a408e9e into apache:master Dec 20, 2022

michaeljmarshall added release/2.11.1 release/2.10.4 labels Dec 20, 2022

lhotari reviewed Dec 20, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java Show resolved Hide resolved

lhotari mentioned this pull request Dec 20, 2022

[improve][broker] Omit making a copy of CommandAck when there are no broker interceptors #18997

Merged

4 tasks

lhotari added the release/2.9.5 label Dec 20, 2022

lhotari mentioned this pull request Dec 20, 2022

[improve][broker] Add javadoc for PulsarDecoder handle* method parameter lifecycle #18998

Merged

4 tasks

michaeljmarshall added the cherry-picked/branch-2.11 label Dec 21, 2022

michaeljmarshall added the cherry-picked/branch-2.10 label Dec 21, 2022

michaeljmarshall added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Dec 21, 2022

michaeljmarshall deleted the use-final-variable branch January 23, 2023 22:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix][broker] Copy proto command fields into final variables in ServerCnx #18987

[fix][broker] Copy proto command fields into final variables in ServerCnx #18987

michaeljmarshall commented Dec 19, 2022 •

edited

Loading

codecov-commenter commented Dec 19, 2022 •

edited

Loading

lhotari commented Dec 20, 2022

michaeljmarshall commented Dec 20, 2022

[fix][broker] Copy proto command fields into final variables in ServerCnx #18987

[fix][broker] Copy proto command fields into final variables in ServerCnx #18987

Conversation

michaeljmarshall commented Dec 19, 2022 • edited Loading

Motivation

Modifications

Verifying this change

Documentation

Matching PR in forked repository

codecov-commenter commented Dec 19, 2022 • edited Loading

Codecov Report

lhotari commented Dec 20, 2022

michaeljmarshall commented Dec 20, 2022

michaeljmarshall commented Dec 19, 2022 •

edited

Loading

codecov-commenter commented Dec 19, 2022 •

edited

Loading