[Reviewer: AJH] Sproutlet API change #1754

sathiyan-sivathas · 2017-02-27T09:32:51Z

This PR implements the Sproutlet API change that I sent you an email about.

The changes:

Split out 100 trying handling into a separate function.
Send 200 OKs to CANCELs between sproutlet wrappers.
Send ACKs to negative responses between sproutlet wrappers.
Add an observation API that sproutlets can use.

Conflicts: modules/pjsip src/scscfsproutlet.cpp

AlexHockey

I've reviewed. Unfortunately I think there are a few high level issues with the code.

The wrappers behave differently depending on whether they're at the start or end of the sproutlet graph. This bound to give inconsistent behavior. Each wrapper should behave the same way, and the UASTsx should be responsible for dealing with the ends of the graph.
In one place you've got the wrappers driving the scheduling, which is backwards because the wrappers are the things that get scheduled.
You've got the absorb ACKs flag in the wrong place IMO.

Let me know if you'd like to discuss any of these issues further.

A few other minor comments inline.

AlexHockey · 2017-02-27T12:22:05Z

include/basicproxy.h

+    /// If flag is set, any ACKs generated in the sproutlet wrapper will be
+    /// absorbed. This is so that we do not send ACKs to negative responses off
+    /// the box.
+    bool _absorb_acks;


This shouldn't live in the basic proxy. It is only a problem because the sproutlet proxy is generating ACKs to negative responses itself.

AlexHockey · 2017-02-27T12:25:10Z

include/sproutlet.h

  virtual void on_rx_response(pjsip_msg* rsp, int fork_id) { send_response(rsp); }

-  /// Called when a response has been transmitted on the transaction.
+  /// Called if a 100 Trying response is received on the transaction. If the


Trailing sentence.

AlexHockey · 2017-02-27T12:28:52Z

include/sproutlet.h

-  /// Called when a response has been transmitted on the transaction.
+  /// Called if a 100 Trying response is received on the transaction. If the
+  /// These responses are only sent by the wrappers, so we shouldn't call
+  /// ever call send_response on them.


This is an API, so you need to talk in terms that the API user will understand. The concept of the wrapper is not exposed on the API so you shouldn't talk about it. I would say something like "This is a notification that a 100 trying has been received. The sproutlet proxy handles these automatically. The Sproutlet Tsx should not forward the response on (i.e. should not call send_request).

AlexHockey · 2017-02-27T12:30:54Z

src/basicproxy.cpp

      pjsip_endpt_schedule_timer(stack_data.endpt, &(_trying_timer), &delay);
    }
+
+    // Sproutlets may generate local ACKs to negative responses, we should not


You shouldn't be talking about sproutlets in the basic proxy.

AlexHockey · 2017-02-27T12:31:09Z

src/basicproxy.cpp

    enter_context();
+
+    // For ACK transactions, we should not absorb ACKS that are generated by
+    // the sproutlets.


AlexHockey · 2017-02-27T12:49:25Z

src/sproutletproxy.cpp

+                                             rsp->msg);
+    ++_pending_sends;
+    tx_request(ack, fork_id);
+    _proxy_tsx->schedule_requests();


I don't think it's right for the wrapper to tell the UASTsx when to schedule requests. In fact, rx_response can be called from schedule_requests so this can cause arbitrary levels of recursion so this is definitely wrong.

AlexHockey · 2017-02-27T12:53:23Z

src/sproutletproxy.cpp

+  // do this id this response has been passed to us directly from a UACTsx since
+  // PJSIP will have sent and ACK already.
+  if ((rsp->msg->line.status.code >= 300) &&
+      (!client_rsp))


So if we only do this on a non-client response, doesn't think mean that the last sproutlet in the chain doesn't observe a -ve ACK being sent? If that's true, that's a problem.

AlexHockey · 2017-02-27T12:58:48Z

src/sproutletproxy.cpp

 {
  TRC_VERBOSE("%s received CANCEL request", _id.c_str());
+
+  // If this isn't the root sproutlet, send an immediate 200 OK to the CANCEL.


Again, the fact we're doing something different at the end of the chain is wrong.

AlexHockey · 2017-02-27T12:59:45Z

src/sproutletproxy.cpp

  }

+  // LCOV_EXCL_START - Sproutlets don't forward 100 Trying responses so we
+  // should never hit this code. But let's keep it here just in case.


So if we hit this code it implies a sproutlet is misusing the API. That is something that could totally happen, so let's add a UT for it and remove the coverage exclusions.

AlexHockey · 2017-02-27T13:00:24Z

src/ut/mock_sproutlet.h

+  MOCK_METHOD2(obs_tx_request, void(pjsip_msg*, int));
  MOCK_METHOD2(on_rx_response, void(pjsip_msg*, int));
-  MOCK_METHOD1(on_tx_response, void(pjsip_msg*));
+  MOCK_METHOD1(obs_tx_response, void(pjsip_msg*));


Might as well add mocks for the other new methods you've added at the same time.

sathiyan-sivathas · 2017-02-28T14:48:43Z

I've applied all the markups - the only outstanding issue is the one about ACRs which I've emailed you about separately. Please could you re-review?

On the max-forwards question - I changed the ordering to store the received request without decrementing the max-forwards because the upstream sproutlet uses the same pointer to see the request that it sent downstream. If we decrement max-forwards on receiving a request then the ACK generated to any negative responses will have the wrong mf header. This becomes a problem if the upstream sproutlet sends a request with mf=1 and we decrement it to 0 on receiving it downstream. Then any ACK to a negative response that is generated upstream will have mf=0 and will be dropped.

sathiyan-sivathas · 2017-03-02T10:27:28Z

A few more changes. I have:

Added a flag tsx_mgmt to indicate to the obs_* callbacks when they are being passed a transaction management message.
Moved the 100 Trying generation to the sproutlet wrappers.
Covered some edge cases e.g. send ACK to negative response from UASTsx to the root sproutlet, and 200 OK to CANCEL from the UASTsx to the last sproutlet in the chain.

I've done some initial testing for LI and all the messages we need seem to be available. Please review.

Conflicts: src/scscfsproutlet.cpp

AlexHockey

Generally looks good - nice one. A few comments inline.

AlexHockey · 2017-03-02T18:30:02Z

include/sproutlet.h

+  ///
+  /// @param  req          - The received request.
+  /// @param  tsx_mgmt     - Whether this is a transaction management message
+  ///                        e.g. CANCEL, 200 OK to CANCEL, 100 Trying, ACK to


It's occurred to me that a really nice way of expressing this is that the flag is set to true when observing

a received message that will not be passed to on_rx_*

a transmitted message that was not generated by the sproutlet tsx calling send_*

Then say that this covers CANCEL, CANCEL 200, 100, and -ve ACK. I would probably put that in a separate comment before any of these methods.

AlexHockey · 2017-03-02T18:31:49Z

include/sproutlet.h

  ///                        was triggered by an error or timeout.
  virtual void on_rx_cancel(int status_code, pjsip_msg* cancel_req) {}

+  /// Called when a request is received by the sproutlet wrapper.


Dude, avoid talking about the sproutlet wrapper! You could say something like "Called when a request is received at this sproutlet Tsx".

Same comment on the rest of the obs_ methods too.

AlexHockey · 2017-03-02T18:36:40Z

include/pjutils.h

                             pjsip_tx_data* tdata,
                             int reason_code);

+pjsip_tx_data* create_ack(pjsip_endpoint* endpt,


Generally you should be adding Doxygen comments to any new functions you add (these are the comments that start with a /// and have the @params and @return handles in them).

It's important here because it's not clear whether this function can create any ACK, or just a -ve ACK (I thought it was the latter, but I might be wrong).

AlexHockey · 2017-03-02T18:40:08Z

src/ut/mock_sproutlet.h

  MOCK_METHOD2(on_rx_response, void(pjsip_msg*, int));
-  MOCK_METHOD1(on_tx_response, void(pjsip_msg*));
+  MOCK_METHOD1(obs_rx_response, void(pjsip_msg*, int));
+  MOCK_METHOD1(obs_tx_response, void(pjsip_msg*));


These mock signatures look wrong to me.

AlexHockey · 2017-03-02T18:40:26Z

src/scscfsproutlet.cpp

  return reason;
 }

+<<<<<<< HEAD


Conflict marker?? How does this code compile?

AlexHockey · 2017-03-02T19:26:27Z

src/sproutletproxy.cpp

+    // This is an ACK to a final response so disconnect the sproutlets.
+    _dmap_sproutlet.erase(std::make_pair(upstream, fork_id));
+    UMap::iterator j = _umap.find((void*)downstream);
+    _umap.erase(j);


Worth commonalizing this code?

AlexHockey · 2017-03-02T19:30:39Z

src/sproutletproxy.cpp

+  {
+    --mf_hdr->ivalue;
+  }
+


Hmm. When do we add the message to the list of messages that the wrapper owns? If we make any changes to it between storing it and passing it into the on_rx_* methods then the message passed to on_rx_* is different from messages returned by original_request, which feels like an issue.

AlexHockey · 2017-03-02T19:32:22Z

src/sproutletproxy.cpp

-      (_forks[fork_id].state.tsx_state == PJSIP_TSX_STATE_CALLING))
+
+  int status_code = rsp->msg->line.status.code;
+  if (status_code == PJSIP_SC_TRYING)


Doesn't this need to cover CANCEL 200s as well?

Ah, I see you handle this below. It would be clearer to put all the observation stuff in one place, even if it means repeating some if tests.

AlexHockey · 2017-03-02T19:33:43Z

src/sproutletproxy.cpp

+  if (PJSIP_MSG_CSEQ_HDR(rsp->msg)->method.id == PJSIP_CANCEL_METHOD)
+  {
+    _sproutlet_tsx->obs_rx_response(rsp->msg, fork_id, true);
+    pjsip_tx_data_dec_ref(rsp);


Don't we also need to drop our reference to a 100 trying?

Maybe you need to do this further down for 100 tryings. But in that case I would move both the 100 and 200 CANCEL processing further down.

AlexHockey · 2017-03-02T19:36:54Z

src/sproutletproxy.cpp

  }

+  // Sproutlets don't forward 100 Trying responses so we should never hit this
+  // code. But let's keep it here just in case.


You've missed the point. even though they shouldn't, sproutlet's can forward 100 trying responses (there is nothing stopping them) so we need to keep this code.

Sathiyan Sivathas added 8 commits February 13, 2017 11:43

Add on_rx_trying callback

5583f60

Add observation API

08d237b

Send 200 OK to CANCEL and negative ACKs between sproutlets

dc746b1

Create ACK from msg

59be268

Decrement max-forwards after cloning the message

4894d65

Fix memory leak

92fb6a5

Take latest PJSIP

da0d644

Merge remote-tracking branch 'origin/dev' into sproutlet-api-change

9f813ac

Conflicts: modules/pjsip src/scscfsproutlet.cpp

sathiyan-sivathas assigned AlexHockey Feb 27, 2017

AlexHockey suggested changes Feb 27, 2017

View reviewed changes

AlexHockey assigned sathiyan-sivathas and unassigned AlexHockey Feb 27, 2017

Markups

8f06221

sathiyan-sivathas assigned AlexHockey and unassigned sathiyan-sivathas Feb 28, 2017

AlexHockey assigned sathiyan-sivathas and unassigned AlexHockey Feb 28, 2017

Sathiyan Sivathas added 4 commits March 1, 2017 09:09

Make behaviour of transaction related messages consistent

7f2dd3a

Merge remote-tracking branch 'origin/dev' into sproutlet-api-change

ba373ee

Fixes from live test

48f465a

Update comments

5f10114

sathiyan-sivathas assigned AlexHockey and unassigned sathiyan-sivathas Mar 2, 2017

Sathiyan Sivathas added 3 commits March 2, 2017 15:47

Store off request earlier

61d79c3

Reordering

177776f

Merge branch 'dev' into sproutlet-api-change

cba4c47

Conflicts: src/scscfsproutlet.cpp

AlexHockey suggested changes Mar 2, 2017

View reviewed changes

AlexHockey removed their assignment Mar 2, 2017

AlexHockey assigned sathiyan-sivathas Mar 2, 2017

Sathiyan Sivathas added 4 commits March 3, 2017 16:01

Markups

50a6850

Update comment

25d456a

Add back scheduling code

1924651

Don't allow 100 Trying forwarding

1530017

[Reviewer: AJH] Sproutlet API change #1754

Are you sure you want to change the base?

[Reviewer: AJH] Sproutlet API change #1754

Uh oh!

Conversation

sathiyan-sivathas commented Feb 27, 2017

Uh oh!

AlexHockey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sathiyan-sivathas commented Feb 28, 2017

Uh oh!

sathiyan-sivathas commented Mar 2, 2017

Uh oh!

AlexHockey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants