Skip to content

[fix][broker] Avoid storing MessageMetadata instances returned by peekMessageMetadata - #15983

Merged
BewareMyPower merged 1 commit into
apache:masterfrom
BewareMyPower:bewaremypower/fix-multi-consumer-dispatcher-metadata-bug
Jun 10, 2022
Merged

[fix][broker] Avoid storing MessageMetadata instances returned by peekMessageMetadata#15983
BewareMyPower merged 1 commit into
apache:masterfrom
BewareMyPower:bewaremypower/fix-multi-consumer-dispatcher-metadata-bug

Conversation

@BewareMyPower

Copy link
Copy Markdown
Contributor

Motivation

#15967 removed the EntryWrapper,
which holds an Entry instance that is never used. Instead, after the
refactoring, the MessageMetadata array is useless, see

final MessageMetadata[] metadataArray = entries.stream()
.map(entry -> Commands.peekMessageMetadata(entry.getDataBuffer(), subscription.toString(), -1))
.toArray(MessageMetadata[]::new);

Each MessageMetadata instance in the array is returned by
peekMessageMetadata, whose returned value references a thread local
object Commands#LOCAL_MESSAGE_METADATA. It brings a problem that if
multiple entries were read, all MessageMetadata elements in the array
reference the same object.

However, accidentally, the wrong invocation of Optional#orElse saves
it. See

final MessageMetadata msgMetadata = optMetadataArray.map(metadataArray -> metadataArray[metadataIndex])
.orElse(Commands.peekMessageMetadata(metadataAndPayload, subscription.toString(), -1));

Each time peekMessageMetadata is called, the thread local message
metadata will be updated. Unlike orElseGet, the expression in orElse
is always called no matter if the optional is empty.

This behavior change increases the invocations count of
peekMessageMetadata and the metadataArray cache became redundant.

Modifications

  • Use orElseGet instead of orElse in AbstractBaseDispatcher.
  • Add a new static method Commands#peekAndCopyMessageMetadata that
    returns a MessageMetadata instance allocated from heap memory.
  • Call peekAndCopyMessageMetadata to cache all message metadata
    instances in PersistentDispatcherMultipleConsumers.

Verifying this change

  • Make sure that the change passes the CI checks.

It's hard to add tests. As I've explained before, #15967 only degrades the
performance and doesn't affect the correctness.

Documentation

Check the box below or label this PR directly.

Need to update docs?

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete

…peekMessageMetadata`

### Motivation

apache#15967 removed the `EntryWrapper`,
which holds an `Entry` instance that is never used. Instead, after the
refactoring, the `MessageMetadata` array is useless, see
https://github.com/apache/pulsar/blob/298a573295f845e46f8a55cee366b6db63e997c2/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java#L517-L519

Each `MessageMetadata` instance in the array is returned by
`peekMessageMetadata`, whose returned value references a thread local
object `Commands#LOCAL_MESSAGE_METADATA`. It brings a problem that if
multiple entries were read, all `MessageMetadata` elements in the array
reference the same object.

However, accidentally, the wrong invocation of `Optional#orElse` saves
it. See
https://github.com/apache/pulsar/blob/298a573295f845e46f8a55cee366b6db63e997c2/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java#L133-L134

Each time `peekMessageMetadata` is called, the thread local message
metadata will be updated. Unlike `orElseGet`, the expression in `orElse`
is always called no matter if the optional is empty.

This behavior change increases the invocations count of
`peekMessageMetadata` and the `metadataArray` cache became redundant.

### Modifications

- Use `orElseGet` instead of `orElse` in `AbstractBaseDispatcher`.
- Add a new static method `Commands#peekAndCopyMessageMetadata` that
  returns a `MessageMetadata` instance allocated from heap memory.
- Call `peekAndCopyMessageMetadata` to cache all message metadata
  instances in `PersistentDispatcherMultipleConsumers`.

### Verifying this change

- [ ] Make sure that the change passes the CI checks.

It's hard to add tests. As I've explained before, apache#15967 only degrades the
performance and doesn't affect the correctness.

### Documentation

Check the box below or label this PR directly.

Need to update docs?

- [ ] `doc-required`
(Your PR needs to update docs and you will update later)

- [x] `doc-not-needed`
(Please explain why)

- [ ] `doc`
(Your PR contains doc changes)

- [ ] `doc-complete`
@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Jun 8, 2022
@BewareMyPower
BewareMyPower requested a review from hangc0276 June 8, 2022 14:40
@BewareMyPower BewareMyPower added type/bug The PR fixed a bug or issue reported a bug area/broker labels Jun 8, 2022
@codelipenghui codelipenghui added this to the 2.11.0 milestone Jun 8, 2022
@BewareMyPower

Copy link
Copy Markdown
Contributor Author

I tried to add the test, but it looks harder than I thought. The only way is to verify the invocation count of the staticCommands#peekMessageMetadata method.

A simple way to expose the problem is reverting the changes of PersistentDispatcherMultipleConsumers.java in this PR and running the following test in FilterEntryTest.

    @Test
    public void testMultiEntriesInMultiConsumerDispatcher() throws Exception {
        String topic = "persistent://prop/ns-abc/topic" + UUID.randomUUID();
        String subName = "sub";

        Consumer<String> consumer = pulsarClient.newConsumer(Schema.STRING)
                .topic(topic)
                // PersistentDispatcherMultipleConsumers will be used for Shared subscription
                .subscriptionType(SubscriptionType.Shared)
                .subscriptionName(subName)
                .subscribe();
        consumer.close();
        PersistentSubscription subscription = (PersistentSubscription) pulsar.getBrokerService()
                .getTopicReference(topic).get().getSubscription(subName);
        Dispatcher dispatcher = subscription.getDispatcher();
        Field field = AbstractBaseDispatcher.class.getDeclaredField("entryFilters");
        field.setAccessible(true);
        NarClassLoader narClassLoader = mock(NarClassLoader.class);

        final List<Long> sequenceIdList = Collections.synchronizedList(new ArrayList<>());
        EntryFilter filter = new EntryFilter() {

            @Override
            public FilterResult filterEntry(Entry entry, FilterContext context) {
                sequenceIdList.add(context.getMsgMetadata().getSequenceId());
                return FilterResult.ACCEPT;
            }

            @Override
            public void close() {
            }
        };
        EntryFilterWithClassLoader loader = spyWithClassAndConstructorArgs(
                EntryFilterWithClassLoader.class, filter, narClassLoader);
        field.set(dispatcher, ImmutableList.of(loader));

        final int numMessages = 10;

        @Cleanup
        Producer<String> producer = pulsarClient.newProducer(Schema.STRING)
                .enableBatching(false)
                .topic(topic)
                .create();
        for (int i = 0; i < numMessages; i++) {
            producer.newMessage().value("msg-" + i).send();
        }

        consumer = pulsarClient.newConsumer(Schema.STRING)
                .topic(topic)
                .subscriptionInitialPosition(SubscriptionInitialPosition.Earliest)
                // PersistentDispatcherMultipleConsumers will be used for Shared subscription
                .subscriptionType(SubscriptionType.Shared)
                .subscriptionName(subName)
                .subscribe();
        while (true) {
            Message<String> msg = consumer.receive(1, TimeUnit.SECONDS);
            if (msg == null) {
                break;
            }
        }

        assertEquals(LongStream.range(0, numMessages).boxed().collect(Collectors.toList()), sequenceIdList);
    }

@BewareMyPower
BewareMyPower merged commit 36690f5 into apache:master Jun 10, 2022
@BewareMyPower
BewareMyPower deleted the bewaremypower/fix-multi-consumer-dispatcher-metadata-bug branch June 10, 2022 07:12
eolivelli pushed a commit to datastax/pulsar that referenced this pull request Sep 16, 2022
…peekMessageMetadata` (apache#15983)

apache#15967 removed the `EntryWrapper`,
which holds an `Entry` instance that is never used. Instead, after the
refactoring, the `MessageMetadata` array is useless, see
https://github.com/apache/pulsar/blob/298a573295f845e46f8a55cee366b6db63e997c2/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java#L517-L519

Each `MessageMetadata` instance in the array is returned by
`peekMessageMetadata`, whose returned value references a thread local
object `Commands#LOCAL_MESSAGE_METADATA`. It brings a problem that if
multiple entries were read, all `MessageMetadata` elements in the array
reference the same object.

However, accidentally, the wrong invocation of `Optional#orElse` saves
it. See
https://github.com/apache/pulsar/blob/298a573295f845e46f8a55cee366b6db63e997c2/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java#L133-L134

Each time `peekMessageMetadata` is called, the thread local message
metadata will be updated. Unlike `orElseGet`, the expression in `orElse`
is always called no matter if the optional is empty.

This behavior change increases the invocations count of
`peekMessageMetadata` and the `metadataArray` cache became redundant.

- Use `orElseGet` instead of `orElse` in `AbstractBaseDispatcher`.
- Add a new static method `Commands#peekAndCopyMessageMetadata` that
  returns a `MessageMetadata` instance allocated from heap memory.
- Call `peekAndCopyMessageMetadata` to cache all message metadata
  instances in `PersistentDispatcherMultipleConsumers`.

It's hard to add tests. As I've explained before, apache#15967 only degrades the
performance and doesn't affect the correctness.

(cherry picked from commit 36690f5)
eolivelli pushed a commit to datastax/pulsar that referenced this pull request Sep 16, 2022
…peekMessageMetadata` (apache#15983)

apache#15967 removed the `EntryWrapper`,
which holds an `Entry` instance that is never used. Instead, after the
refactoring, the `MessageMetadata` array is useless, see
https://github.com/apache/pulsar/blob/298a573295f845e46f8a55cee366b6db63e997c2/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java#L517-L519

Each `MessageMetadata` instance in the array is returned by
`peekMessageMetadata`, whose returned value references a thread local
object `Commands#LOCAL_MESSAGE_METADATA`. It brings a problem that if
multiple entries were read, all `MessageMetadata` elements in the array
reference the same object.

However, accidentally, the wrong invocation of `Optional#orElse` saves
it. See
https://github.com/apache/pulsar/blob/298a573295f845e46f8a55cee366b6db63e997c2/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java#L133-L134

Each time `peekMessageMetadata` is called, the thread local message
metadata will be updated. Unlike `orElseGet`, the expression in `orElse`
is always called no matter if the optional is empty.

This behavior change increases the invocations count of
`peekMessageMetadata` and the `metadataArray` cache became redundant.

- Use `orElseGet` instead of `orElse` in `AbstractBaseDispatcher`.
- Add a new static method `Commands#peekAndCopyMessageMetadata` that
  returns a `MessageMetadata` instance allocated from heap memory.
- Call `peekAndCopyMessageMetadata` to cache all message metadata
  instances in `PersistentDispatcherMultipleConsumers`.

It's hard to add tests. As I've explained before, apache#15967 only degrades the
performance and doesn't affect the correctness.

(cherry picked from commit 36690f5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/broker doc-not-needed Your PR changes do not impact docs type/bug The PR fixed a bug or issue reported a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants