-
Notifications
You must be signed in to change notification settings - Fork 141
Fix[BMQ]: unsafe schemalearner use #618
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
a454b0d
to
91d01b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build 2484 of commit 91d01b5 has completed with FAILURE
@@ -60,6 +60,7 @@ | |||
// BMQ | |||
|
|||
#include <bmqa_queueid.h> | |||
#include <bmqp_messageproperties.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can. No nested MessageProperties::Schema
though, now it has to be MessageProperties_Schema
|
||
const SchemaPtr& result = contextHandle->d_schema_sp; | ||
|
||
if (!result) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we assume that it's an unlikely scenario? Or is it better to evaluate the expression without such assumptions?
MessageProperties mps(d_allocator_p); | ||
int rc = mps.streamIn(blob, input, result); | ||
|
||
if (rc == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If rc != 0
we will silently return an empty SchemaPtr
. Should we log something or skip the message?
bmqp::MessageProperties::SchemaPtr schema = schemaLearner().learn( | ||
schemaLearner().createContext(queueId), | ||
bmqp::MessagePropertiesInfo(info.d_header), | ||
bdlbb::Blob()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
91d01b5
to
f0622b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build 2518 of commit f0622b7 has completed with FAILURE
f0622b7
to
bbb706a
Compare
|
||
if (mps.streamIn(appData, input.isExtended()) == 0) { | ||
// Learn new schema. | ||
*schemaHolder = schema = mps.makeSchema(d_allocator_p); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same concern about the number of SchemaPtr
copies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build 2657 of commit 5b46679 has completed with SUCCESS
7fc32f5
to
2ea8627
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build 2676 of commit 2ea8627 has completed with FAILURE
2ea8627
to
efcc512
Compare
src/groups/bmq/bmqa/bmqa_message.h
Outdated
@@ -117,6 +121,8 @@ struct MessageImpl { | |||
/// SubscriptionHandle this message is associated with | |||
bmqt::SubscriptionHandle d_subscriptionHandle; | |||
|
|||
bsl::shared_ptr<const bmqp::MessageProperties_Schema> d_schema_sp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a private component of MessageProperties
, we should probably be referring to it through the typedef
bsl::shared_ptr<const bmqp::MessageProperties_Schema> d_schema_sp; | |
bmqp::MessageProperties::SchemaPtr d_schema_sp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #618 (comment)
We can return to that version, sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build 2723 of commit efcc512 has completed with SUCCESS
Hi @dorjesinpo, do you want to make any more changes to this PR? |
Hi. Other than the typedef change, no. Was waiting for the next round of your review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last comment
bmqp::EventUtilQueueInfo(msgIterator.header(), | ||
subQueueInfos[0].id(), | ||
msgIterator.applicationDataSize(), | ||
schema)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still extra schema
copies here, can do the same change you did in another file:
bmqp::MessageProperties::SchemaPtr* schema_p = 0;
schema_p = d_schemaLearner.observe(d_schemaLearner.createContext(queueId),
input);
if (schema_p) {
if (schema_p->get() == 0) {
// Learn new Schema by reading all MessageProperties.
bmqp::MessageProperties mps(d_allocator_p);
if (mps.streamIn(d_appData, input.isExtended()) == 0) {
// Learn new schema.
*schema_p = mps.makeSchema(d_allocator_p);
}
}
}
Signed-off-by: dorjesinpo <[email protected]>
Signed-off-by: dorjesinpo <[email protected]>
Signed-off-by: dorjesinpo <[email protected]>
Signed-off-by: dorjesinpo <[email protected]>
Signed-off-by: dorjesinpo <[email protected]>
efcc512
to
ef0bd0d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build 2742 of commit ef0bd0d has completed with FAILURE
bmqp::MessageProperties::SchemaPtr schema; | ||
|
||
if (schema_p) { | ||
if (!schema_p->get() == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this !
a bug?
if (!schema_p->get() == 0) { | |
if (schema_p->get() == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this
!
a bug?
it is! Amended
Signed-off-by: dorjesinpo <[email protected]>
ef0bd0d
to
b7531cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build 2748 of commit b7531cc has completed with FAILURE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
BSLS_ASSERT_SAFE(d_rawEvent.isPushEvent()); | ||
BSLS_ASSERT_SAFE(0 <= position); | ||
BSLS_ASSERT_SAFE(static_cast<int>(d_correlationIds.size()) > position); | ||
BSLS_ASSERT_SAFE(static_cast<int>(d_contexts.size()) > position); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would prefer to reorder so the smaller value is on the left
BSLS_ASSERT_SAFE(static_cast<int>(d_contexts.size()) > position); | |
BSLS_ASSERT_SAFE(position < static_cast<int>(d_contexts.size())); |
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.