Fix[BMQ]: unsafe schemalearner use #618

dorjesinpo · 2025-02-19T16:32:31Z

Cannot use bmqp::SchemaLearner in both FSM and event threads.
Instead, call the leaner in FSM and cache the result (schema) in the generated event.

Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>

bmq-oss-ci

Build 2484 of commit 91d01b5 has completed with FAILURE

678098 · 2025-02-24T14:57:59Z

src/groups/bmq/bmqa/bmqa_message.h

@@ -60,6 +60,7 @@
 // BMQ

 #include <bmqa_queueid.h>
+#include <bmqp_messageproperties.h>


The only change in this header is this new field:
bmqp::MessageProperties::SchemaPtr d_schema_sp

It is a pointer type, so we don't need to know the class sizeof in advance. We can replace include to a heavy header with a forward declaration to this type. At some point in the future we might want to optimize includes to achieve a better compilation time.

If you think it's helpful, you can make this change now.

678098 · 2025-02-24T15:51:10Z

src/groups/bmq/bmqimp/bmqimp_event.h

@@ -251,8 +259,7 @@ class Event {
    // has been constructed in its buffer.  We
    // cannot rely on 'd_msgEventMode'

-    bsl::vector<bsl::pair<bmqt::CorrelationId, unsigned int> >
-        d_correlationIds;
+    bsl::vector<MessageContext> d_contexts;


Need to update the comment below

678098 · 2025-02-24T16:00:51Z

src/groups/bmq/bmqimp/bmqimp_event.h


-    return d_correlationIds[position].second;
+    return d_contexts[position];


Less code possible

We can turn position arg to type size_t (it allows to remove the check 0 <= position.
And use vector.at(size_t pos) that has another range check with performance hint:
https://github.com/bloomberg/bde/blob/2c7ef378b00a779198ef4bfc774ce835d741c545/groups/bsl/bslstl/bslstl_vector.h#L2496

678098 · 2025-02-24T16:23:06Z

src/groups/bmq/bmqimp/bmqimp_event.h

@@ -811,8 +803,11 @@ inline void Event::addCorrelationId(const bmqt::CorrelationId& correlationId,
    // BSLS_ASSERT_SAFE(messageEventMode() == MessageEventMode::e_READ);
    // BSLS_ASSERT_SAFE(d_rawEvent.isAckEvent());

-    d_correlationIds.push_back(
-        bsl::make_pair(correlationId, subscriptionHandleId));
+    d_contexts.resize(d_contexts.size() + 1);


Here, we construct MessageContext with a default constructor, then reassign all fields one by one. Might be better to emplace_back:
https://github.com/bloomberg/bde/blob/65f2de5338d10f6a947357af66b100b1490616bb/groups/bsl/bslstl/bslstl_vector.h#L3681

678098 · 2025-02-24T16:25:12Z

src/groups/bmq/bmqimp/bmqimp_event.h

+    void addContext(const bmqt::CorrelationId& correlationId,
+                    unsigned int               subscriptionHandleId =
+                        bmqt::SubscriptionHandle::k_INVALID_HANDLE_ID,
+                    bmqp::MessageProperties::SchemaPtr schema =


New arg schema not mentioned in docs

678098 · 2025-02-24T17:00:07Z

src/groups/bmq/bmqp/bmqp_schemalearner.cpp

+
+    const SchemaPtr& result = contextHandle->d_schema_sp;
+
+    if (!result) {


Can we assume that it's an unlikely scenario? Or is it better to evaluate the expression without such assumptions?

678098 · 2025-02-24T17:01:12Z

src/groups/bmq/bmqp/bmqp_schemalearner.cpp

+        MessageProperties mps(d_allocator_p);
+        int               rc = mps.streamIn(blob, input, result);
+
+        if (rc == 0) {


If rc != 0 we will silently return an empty SchemaPtr. Should we log something or skip the message?

678098 · 2025-02-24T17:07:33Z

src/groups/bmq/bmqp/bmqp_messageproperties.h

@@ -99,6 +99,8 @@ class MessageProperties_Schema {
                             bslma::Allocator*        basicAllocator);
    MessageProperties_Schema(const MessageProperties_Schema& other);

+    ~MessageProperties_Schema();


Let's also add BSLS_KEYWORD_FINAL to the class head, since we declared a non-virtual destructor?

class MessageProperties_Schema BSLS_KEYWORD_FINAL {

678098 · 2025-02-24T17:08:15Z

src/groups/bmq/bmqp/bmqp_schemalearner.cpp

@@ -45,6 +45,8 @@ struct SchemaLearner::SchemaHandle {
    // If 'false', needs new translation.

    SchemaHandle(SchemaIdType id);
+
+    ~SchemaHandle();


Let's also add BSLS_KEYWORD_FINAL to the class head, since we declared a non-virtual destructor?

struct SchemaLearner::SchemaHandle BSLS_KEYWORD_FINAL {

678098 · 2025-02-24T17:18:49Z

src/groups/bmq/bmqimp/bmqimp_queuemanager.cpp

+    bmqp::MessageProperties::SchemaPtr schema = schemaLearner().learn(
+        schemaLearner().createContext(queueId),
+        bmqp::MessagePropertiesInfo(info.d_header),
+        bdlbb::Blob());


Looking at learn implementation:

SchemaLearner::SchemaPtr SchemaLearner::learn(Context& context, const MessagePropertiesInfo& input, const bdlbb::Blob& blob) { const SchemaIdType inputId = input.schemaId(); if (!isPresentAndValid(inputId)) { // Nothing to do return SchemaPtr(); // RETURN }

We have an early return if schemaId is not present or valid. Does it happen for all messages that don't have message properties? If it can happen that frequently, we do a lot of unnecessary work here:

Pass args and call schemaLearner().createContext(queueId)

Within schemaLearner().createContext(queueId) make lookup in a map (we don't use its result)

Construct bdlbb::Blob() that we don't use

Pass args and call SchemaLearner::learn just to return early

Do you think it's worth to avoid these extra operations if we don't have schemaId?

dorjesinpo added the bug Something isn't working label Feb 19, 2025

dorjesinpo requested a review from a team as a code owner February 19, 2025 16:32

dorjesinpo added 2 commits February 19, 2025 12:32

Fixing thread-unsafe QueueMgr and SchemaLeaneruse

2bda5bd

Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>

Fixing thread-unsafe SchemaLeaner use

91d01b5

Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>

dorjesinpo force-pushed the fix/unsafe-scheamalearner-use branch from a454b0d to 91d01b5 Compare February 19, 2025 17:33

bmq-oss-ci bot reviewed Feb 19, 2025

View reviewed changes

dorjesinpo assigned 678098 Feb 19, 2025

dorjesinpo requested a review from 678098 February 19, 2025 18:58

678098 changed the title ~~Fix unsafe scheamalearner use~~ Fix unsafe schemalearner use Feb 24, 2025

678098 changed the title ~~Fix unsafe schemalearner use~~ Fix[BMQ]: unsafe schemalearner use Feb 24, 2025

678098 reviewed Feb 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix[BMQ]: unsafe schemalearner use #618

Fix[BMQ]: unsafe schemalearner use #618

dorjesinpo commented Feb 19, 2025

bmq-oss-ci bot left a comment

678098 Feb 24, 2025

678098 Feb 24, 2025

678098 Feb 24, 2025

678098 Feb 24, 2025

678098 Feb 24, 2025

678098 Feb 24, 2025

678098 Feb 24, 2025

678098 Feb 24, 2025

678098 Feb 24, 2025

678098 Feb 24, 2025


		return d_correlationIds[position].second;
		return d_contexts[position];


		const SchemaPtr& result = contextHandle->d_schema_sp;

		if (!result) {

Fix[BMQ]: unsafe schemalearner use #618

Are you sure you want to change the base?

Fix[BMQ]: unsafe schemalearner use #618

Conversation

dorjesinpo commented Feb 19, 2025

bmq-oss-ci bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment