Feat[MQB]: refactor negotiator #670

emelialei88 · 2025-03-25T13:15:29Z

No description provided.

Copilot

Copilot reviewed 28 out of 30 changed files in this pull request and generated no comments.

Files not reviewed (2)

src/groups/bmq/bmqp/bmqp_ctrlmsg.xsd: Language not supported
src/groups/mqb/mqba/package/mqba.mem: Language not supported

Comments suppressed due to low confidence (2)

src/groups/mqb/mqba/mqba_sessionnegotiator.cpp:1041

The 'initiateOutboundNegotiation' function now returns an int. Please verify that all callers consistently handle non-zero error codes and propagate them appropriately.

int rc = initiateOutboundNegotiation(negotiationContext);

src/groups/mqb/mqba/mqba_authenticator.cpp:46

[nitpick] The 'readCallback' function is currently empty. Consider providing an implementation or adding tests to validate that read events are handled as expected.

void Authenticator::readCallback(const bmqio::Status&           status,

dorjesinpo

Can we make the authenticator a separate PR if it is not used?

The prefix initialConnect appears many times, we may want to rethink the names.
The connect part does not match TCP listen context in which it is used.

We have InitialConnectionHandlerContext and InitialConnectionContext
Potentially, they can be even merged (the existing design has them split which is fine). We may need better names.

dorjesinpo · 2025-04-09T14:46:26Z

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.h

    /// specified `context`.
-    void negotiate(const bsl::shared_ptr<bmqio::Channel>&   channel,
-                   const bsl::shared_ptr<OperationContext>& context);
+    void initialConnect(const bsl::shared_ptr<bmqio::Channel>&   channel,


minor. initialConnect seems to apply to both connect and listen results. If so, the name connect in the context of TCP may imply the connect part and the listen part is less obvious.

What about "handleInitialConnection"?

dorjesinpo · 2025-04-09T14:53:20Z

src/groups/mqb/mqba/mqba_initialconnectionhandler.cpp

+        scheduleRead(initialConnectionContext);
+    }
+    else {
+        if (d_negotiator_mp->negotiateOutboundOrReverse(


We used to have a comment in SessionNegotiator::negotiate for this case // If this is a 'connect' negotiation, this could either represent an.... Is it gone?

No. It resides in SessionNegotiator::negotiateOutboundOrReverse now.

dorjesinpo · 2025-04-09T14:55:58Z

src/groups/mqb/mqba/mqba_initialconnectionhandler.cpp

+        scheduleRead(initialConnectionContext);
+    }
+    else {
+        if (d_negotiator_mp->negotiateOutboundOrReverse(


What are the authentication requirements (going to be) in this connect case?

I would imagine something similar. We read if it's an incoming request, otherwise maybe have a function like authenticateOutboundOrReverse.

dorjesinpo · 2025-04-09T15:07:35Z

src/groups/mqb/mqbnet/mqbnet_initialconnectionhandler.h

+    /// needed, the `initialConnectionCb` may be invoked directly from inside
+    /// the call to `initialConnect()`.
+    virtual void
+    initialConnect(mqbnet::InitialConnectionHandlerContext* context,


See my other comment about initialConnect name

src/groups/mqb/mqbnet/mqbnet_initialconnectioncontext.h

dorjesinpo · 2025-04-09T15:17:03Z

src/groups/mqb/mqba/mqba_initialconnectionhandler.cpp

+        return;  // RETURN
+    }
+
+    switch (context->d_initialConnectionMessage_p.selectionId()) {


decodeNegotiationMessage does not have to cache the decoded message (d_initialConnectionMessage_p) if it is used in the same stack frame.

Do you mean in the decodeNegotiationMessage we shouldn't create an Event?

I meant that decodeNegotiationMessage does not need to assign to d_negotiationMessage (unless we need to keep the last received message in which case the name should be different). The assignment can happen when we ready to pass the handling to the negotiator

dorjesinpo · 2025-04-09T15:20:17Z

src/groups/mqb/mqba/mqba_initialconnectionhandler.h

+    /// `context`, returning 0.  Return a non-zero code on error and
+    /// populate the specified `errorDescription` with a description of the
+    /// error.
+    int decodeNegotiationMessage(bsl::ostream& errorDescription,


When we add authentication, will this method read authentication messages? If so, do we still want to call that negotiation?

changed to decodeInitialConnectionMessage

Copilot

Copilot reviewed 24 out of 25 changed files in this pull request and generated no comments.

Files not reviewed (1)

src/groups/mqb/mqba/package/mqba.mem: Language not supported

Comments suppressed due to low confidence (1)

src/groups/mqb/mqba/mqba_sessionnegotiator.cpp:296

The callback is being passed '*session' (i.e. the object pointed by the shared_ptr) instead of passing the shared_ptr itself. This could lead to undefined behavior if 'session' is empty; please change it to 'session'.

context->d_initialConnectionCb(rc_NO_ADMIN_CALLBACK, error, *session);

src/groups/mqb/mqba/mqba_sessionnegotiator.cpp

dorjesinpo

A suggestion. Why don't we keep the name NegotiationContext. InitialConnectionHandler creates NegotiationContext when it is ready to pass the connection handling to the negotiator. After all, this is the context for negotiator use.
That way, we clearly separate context for negotiation from the context for pre-negotiation phase (authentication).
And we have one less ``InitialConnection` prefix.

So, the chain looks like this TCPSessionFactory_OperationContext -> InitialConnectionHandlerContext (can be shortened to InitialConnectionContext) -> NegotiationContext

src/groups/mqb/mqbnet/mqbnet_initialconnectionhandlercontext.t.cpp

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.h

dorjesinpo · 2025-04-10T19:29:09Z

src/groups/mqb/mqba/mqba_initialconnectionhandler.cpp

+        return;  // RETURN
+    }
+
+    switch (context->d_initialConnectionMessage_p.selectionId()) {


I meant that decodeNegotiationMessage does not need to assign to d_negotiationMessage (unless we need to keep the last received message in which case the name should be different). The assignment can happen when we ready to pass the handling to the negotiator

…ator Signed-off-by: Emelia Lei <[email protected]>

Signed-off-by: Emelia Lei <[email protected]>

… context Signed-off-by: Emelia Lei <[email protected]>

Signed-off-by: Emelia Lei <[email protected]>

…otiate() Signed-off-by: Emelia Lei <[email protected]>

Signed-off-by: Emelia Lei <[email protected]>

… as mqbnet Signed-off-by: Emelia Lei <[email protected]>

Signed-off-by: Emelia Lei <[email protected]>

InitialConnectionContext -> NegotiationContext; InitialConnectionHandlerContext -> InitialConnectionContext Signed-off-by: Emelia Lei <[email protected]>

Signed-off-by: Emelia Lei <[email protected]>

dorjesinpo

Getting there. Few more comments

dorjesinpo · 2025-04-14T16:06:57Z

src/groups/mqb/mqba/mqba_initialconnectionhandler.h

+    bdlmt::FixedThreadPool d_threadPool;
+
+    /// Allocator store to spawn new allocators for sub components.
+    bmqma::CountingAllocatorStore d_allocators;


Is it used anywhere?

dorjesinpo · 2025-04-14T16:15:35Z

src/groups/mqb/mqba/mqba_sessionnegotiator.cpp

@@ -475,7 +404,8 @@ SessionNegotiator::onClientIdentityMessage(bsl::ostream& errorDescription,
    case bmqp_ctrlmsg::ClientType::E_TCPBROKER:
    case bmqp_ctrlmsg::ClientType::E_TCPADMIN: {
        if (clientIdentity.hostName().empty()) {
-            clientIdentity.hostName() = context->d_channelSp->peerUri();
+            clientIdentity.hostName() =
+                context->d_initialConnectionContext_p->channel()->peerUri();


Feel free to ignore. Could have NegotiationContext::channel() accessor

dorjesinpo · 2025-04-14T16:17:36Z

src/groups/mqb/mqba/mqba_sessionnegotiator.cpp

 {
-    // Create a NegotiationContext for that connection


BSLS_ASSERT_SAFE(context)

dorjesinpo · 2025-04-14T16:22:47Z

src/groups/mqb/mqbnet/mqbnet_initialconnectionhandler.t.cpp

+
+// Disable some compiler warning for simplified write of the
+// 'AbstractSessionTestImp'.
+#if defined(BSLS_PLATFORM_CMP_CLANG)


same comment as before, no need for the #pragma. Just make handleInitialConnection not inline

dorjesinpo · 2025-04-14T19:41:36Z

src/groups/mqb/mqba/mqba_initialconnectionhandler.cpp

+        if (rc == rc_CONTINUE_READ) {
+            scheduleRead(context);
+        }
+    }


we need to make sure initialConnectionCompleteCb always gets called. Otherwise, the connection is stuck.

Regarding calling initialConnectionCompleteCb, there may be an option of calling it from InitialConnectionHandler only (never from the Negotiator) if the Negotiator is synchronous. Could make the logic easier to follow.

emelialei88 force-pushed the feat/authn branch from b9509de to 20eb12b Compare April 4, 2025 20:16

emelialei88 changed the title ~~[WIP] Feat/authn: adding protocols for authn~~ Feat[MQB]: adding protocols for authn Apr 4, 2025

emelialei88 force-pushed the feat/authn branch 4 times, most recently from e6b2fad to 0ee460f Compare April 7, 2025 21:30

emelialei88 requested a review from Copilot April 7, 2025 21:50

Copilot AI reviewed Apr 7, 2025

View reviewed changes

dorjesinpo requested changes Apr 9, 2025

View reviewed changes

emelialei88 changed the title ~~Feat[MQB]: adding protocols for authn~~ Feat[MQB]: refactor negotiator Apr 9, 2025

emelialei88 force-pushed the feat/authn branch 2 times, most recently from a01af08 to c6c893c Compare April 9, 2025 17:17

emelialei88 requested review from Copilot and dorjesinpo April 9, 2025 18:18

Copilot AI reviewed Apr 9, 2025

View reviewed changes

emelialei88 assigned emelialei88, dorjesinpo and pniedzielski and unassigned emelialei88 Apr 9, 2025

chrisbeard reviewed Apr 9, 2025

View reviewed changes

src/groups/mqb/mqba/mqba_sessionnegotiator.cpp Show resolved Hide resolved

emelialei88 force-pushed the feat/authn branch 5 times, most recently from 623737d to 07e722a Compare April 10, 2025 18:12

dorjesinpo requested changes Apr 10, 2025

View reviewed changes

emelialei88 force-pushed the feat/authn branch from 2903633 to a18ae1c Compare April 11, 2025 15:24

emelialei88 added 3 commits April 11, 2025 11:24

add class initialConnectionHandler to manage authenticator and negoti…

23054fc

…ator Signed-off-by: Emelia Lei <[email protected]>

take out scheduleRead() from negotiation

08b77f1

Signed-off-by: Emelia Lei <[email protected]>

rename negotiation/negotiator context to initial connection (handler)…

77b0e14

… context Signed-off-by: Emelia Lei <[email protected]>

emelialei88 added 24 commits April 11, 2025 11:24

add ich between negotiator and tcpsessionfactory

946e911

Signed-off-by: Emelia Lei <[email protected]>

remove the use of mqbnet::Negotiator

f54492c

Signed-off-by: Emelia Lei <[email protected]>

adding function negotiateOutboundOrReverse() and moving away from neg…

0b404bb

…otiate() Signed-off-by: Emelia Lei <[email protected]>

move mqba_initialconnectioncontext to mqbnet

2303050

Signed-off-by: Emelia Lei <[email protected]>

add negotiateOutboundOrReverse to be interface of negotiator

9039cf2

Signed-off-by: Emelia Lei <[email protected]>

remove negotiate() interface

9f4b3c4

Signed-off-by: Emelia Lei <[email protected]>

add createSessionOnMsgType to interface; make Negotiator in ICH store…

c298a2a

… as mqbnet Signed-off-by: Emelia Lei <[email protected]>

polish negotiateOutboundOrReverse

73965da

Signed-off-by: Emelia Lei <[email protected]>

createSessionOnMsgType swap args

3f13c69

Signed-off-by: Emelia Lei <[email protected]>

add unit tests to interfaces

622da69

Signed-off-by: Emelia Lei <[email protected]>

rename some funtions and variables

2c98eea

Signed-off-by: Emelia Lei <[email protected]>

remove authn

e9b0d7a

Signed-off-by: Emelia Lei <[email protected]>

change d_initialConnectionMessage to d_negotiationMessage

5922893

Signed-off-by: Emelia Lei <[email protected]>

edit comments

f39e744

Signed-off-by: Emelia Lei <[email protected]>

minor fix based on feedback

3d2392b

Signed-off-by: Emelia Lei <[email protected]>

change names

0f65106

InitialConnectionContext -> NegotiationContext; InitialConnectionHandlerContext -> InitialConnectionContext Signed-off-by: Emelia Lei <[email protected]>

change InitialConnectionContext in NegotiationContext to shared_ptr

7a6dc2d

Signed-off-by: Emelia Lei <[email protected]>

change the name of InitialConnectionCompleteCb

bec057d

Signed-off-by: Emelia Lei <[email protected]>

move channel and cb to InitialConnectionContext (step 1 - add)

37482d7

Signed-off-by: Emelia Lei <[email protected]>

remove channel (step 2 - remove)

d90f407

Signed-off-by: Emelia Lei <[email protected]>

remove cb (step 2 - remove)

0888a57

Signed-off-by: Emelia Lei <[email protected]>

clean handleInitialConnection interface (step 2 - remove)

c07094d

Signed-off-by: Emelia Lei <[email protected]>

add InitialConnectionContext to read

37ab640

Signed-off-by: Emelia Lei <[email protected]>

change the relationship of contexts

46d527a

Signed-off-by: Emelia Lei <[email protected]>

emelialei88 force-pushed the feat/authn branch from a18ae1c to 46d527a Compare April 11, 2025 15:24

fix some comments and unit tests

b169709

Signed-off-by: Emelia Lei <[email protected]>

emelialei88 requested a review from dorjesinpo April 11, 2025 16:33

dorjesinpo requested changes Apr 14, 2025

View reviewed changes

dorjesinpo assigned emelialei88 and unassigned dorjesinpo Apr 14, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat[MQB]: refactor negotiator #670

Feat[MQB]: refactor negotiator #670

emelialei88 commented Mar 25, 2025

Copilot AI left a comment

dorjesinpo left a comment

dorjesinpo Apr 9, 2025

emelialei88 Apr 9, 2025

dorjesinpo Apr 9, 2025

emelialei88 Apr 9, 2025

dorjesinpo Apr 9, 2025

emelialei88 Apr 9, 2025

dorjesinpo Apr 9, 2025

dorjesinpo Apr 9, 2025

emelialei88 Apr 9, 2025

dorjesinpo Apr 10, 2025

dorjesinpo Apr 9, 2025

emelialei88 Apr 9, 2025

Copilot AI left a comment

dorjesinpo left a comment

dorjesinpo Apr 10, 2025

dorjesinpo left a comment

dorjesinpo Apr 14, 2025

dorjesinpo Apr 14, 2025

dorjesinpo Apr 14, 2025

dorjesinpo Apr 14, 2025

dorjesinpo Apr 14, 2025

Feat[MQB]: refactor negotiator #670

Are you sure you want to change the base?

Feat[MQB]: refactor negotiator #670

Conversation

emelialei88 commented Mar 25, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

dorjesinpo 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

dorjesinpo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorjesinpo 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