Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ReliableMessageContext::HasPeerRequestedAck should not exist #7339

Closed
bzbarsky-apple opened this issue Jun 2, 2021 · 0 comments · Fixed by #7958
Closed

ReliableMessageContext::HasPeerRequestedAck should not exist #7339

bzbarsky-apple opened this issue Jun 2, 2021 · 0 comments · Fixed by #7958
Assignees
@bzbarsky-apple

Comments

@bzbarsky-apple
Copy link
Contributor

Problem

ReliableMessageContext::HasPeerRequestedAck has the following properties:

  1. Starts out false.
  2. Becomes true if any message is received on the exchange that has the "ack requested" flag set.
  3. Stays true after that.

The only place the flag is checked is ExchangeMessageDispatch::SendMessage to decide whether to piggyback an ack.... but that's the wrong thing to check. It should be checking for a pending ack, not whether we have ever had an ack requested.

Proposed Solution

Change ExchangeMessageDispatch::SendMessage to check IsAckPending and remove the unused and easily misused HasPeerRequestedAck machinery.
@bzbarsky-apple bzbarsky-apple self-assigned this Jun 2, 2021
@bzbarsky-apple bzbarsky-apple mentioned this issue Jun 2, 2021
Merged
@bzbarsky-apple bzbarsky-apple mentioned this issue Jun 18, 2021
Closed
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Jun 26, 2021
@bzbarsky-apple
Don't send piggybacked acks when they are not needed.
932fd30 
We used to set a persistent flag on reliable message contexts that
would cause them to send piggyback acks with all responses after that
point.  Stop doing that.

Also simplify the handling of the IsAckPending state.  It gets set to
true when we set a pending ack id, gets set to false when someone
takes that ack id from us (e.g. to send the ack).  This ensures that
SendStandaloneAckMessage() clears the IsAckPending() state (which it
already did, but that was not very obvious).

Fixes project-chip#7339
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Jun 28, 2021
@bzbarsky-apple
Don't send piggybacked acks when they are not needed.
cdca286 
We used to set a persistent flag on reliable message contexts that
would cause them to send piggyback acks with all responses after that
point.  Stop doing that.

Also simplify the handling of the IsAckPending state.  It gets set to
true when we set a pending ack id, gets set to false when someone
takes that ack id from us (e.g. to send the ack).  This ensures that
SendStandaloneAckMessage() clears the IsAckPending() state (which it
already did, but that was not very obvious).

Fixes project-chip#7339
@bzbarsky-apple bzbarsky-apple mentioned this issue Jun 28, 2021
Merged
woody-apple pushed a commit that referenced this issue Jun 28, 2021
@bzbarsky-apple
Don't send piggybacked acks when they are not needed. (#7958)
233ec0f 
We used to set a persistent flag on reliable message contexts that
would cause them to send piggyback acks with all responses after that
point.  Stop doing that.

Also simplify the handling of the IsAckPending state.  It gets set to
true when we set a pending ack id, gets set to false when someone
takes that ack id from us (e.g. to send the ack).  This ensures that
SendStandaloneAckMessage() clears the IsAckPending() state (which it
already did, but that was not very obvious).

Fixes #7339
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this issue Sep 23, 2021
@bzbarsky-apple
Don't send piggybacked acks when they are not needed. (project-chip#7958
a8c1fe4 
)

We used to set a persistent flag on reliable message contexts that
would cause them to send piggyback acks with all responses after that
point.  Stop doing that.

Also simplify the handling of the IsAckPending state.  It gets set to
true when we set a pending ack id, gets set to false when someone
takes that ack id from us (e.g. to send the ack).  This ensures that
SendStandaloneAckMessage() clears the IsAckPending() state (which it
already did, but that was not very obvious).

Fixes project-chip#7339
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bzbarsky-apple bzbarsky-apple

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Don't send piggybacked acks when they are not needed. bzbarsky-apple/connectedhomeip
1 participant
@bzbarsky-apple