search

openweave / openweave-core

openWeave is a home area network application protocol stack designed to enable asynchronous, symmetric, device-to-device, device-to-mobile and device-to-cloud communications for control path and data path messaging.
Apache License 2.0
233 stars 105 forks source link

Make sure protocol callback is set on the binding object in SubscriptionClient #662

Closed didishe90 closed 3 years ago

didishe90 commented 3 years ago

SubscriptionClient is written in a way that it's not released upon abort. When peer send a "cancel subscription" request, SubscriptionClient is reset to initiated state and is going to be reused by the next subscription attempt. However, SubscriptionClient only sets the protocol callback on its binding upon init(), which doesn't happen when the same subscriptionClient is reused for the second (and new) subscription attempt. In recent tests, we found that some application resets binding during AbortSubscription. In some corner cases, when subscriptionClient is reused for second subscription, protocol callback is not set for its binding. We should double check binding has valid protocol callback when InitiateSubscription() happens. This PR is also a temporary fix to unblock cancel/re-subscribe for some application.

We might also consider release subscriptionClient upon "cancel" to align with this: https://github.com/openweave/openweave-core/blob/9f9d2450ba3308f16cc729324801992d37130813/src/lib/profiles/data-management/Current/SubscriptionClient.h#L446

codecov-commenter commented 3 years ago

Codecov Report

Merging #662 (f74bbc6) into master (2468803) will increase coverage by 15.92%. The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #662       +/-   ##
===========================================
+ Coverage   54.16%   70.09%   +15.92%     
===========================================
  Files         329      484      +155     
  Lines       57161    87455    +30294     
===========================================
+ Hits        30962    61301    +30339     
+ Misses      26199    26154       -45
Impacted Files Coverage Δ
...les/data-management/Current/SubscriptionClient.cpp 84.58% <0.00%> (+76.95%) :arrow_up:
src/test-apps/schema/nest/test/trait/TestETrait.h 55.55% <0.00%> (-44.45%) :arrow_down:
src/lib/profiles/time/WeaveTime.h 60.00% <0.00%> (-40.00%) :arrow_down:
src/lib/core/WeaveCircularTLVBuffer.h 62.50% <0.00%> (-37.50%) :arrow_down:
src/lib/support/logging/WeaveLogging.cpp 56.86% <0.00%> (-19.33%) :arrow_down:
...i/security-support/native/WeaveSecuritySupport.cpp 57.89% <0.00%> (-15.44%) :arrow_down:
...iles/data-management/Current/LoggingManagement.cpp 78.02% <0.00%> (-14.07%) :arrow_down:
src/lib/support/PersistedCounter.cpp 79.24% <0.00%> (-10.12%) :arrow_down:
src/lib/core/WeaveTLVWriter.cpp 85.53% <0.00%> (-7.42%) :arrow_down:
src/lib/profiles/security/WeaveSig.cpp 87.25% <0.00%> (-5.55%) :arrow_down:
... and 280 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2468803...f74bbc6. Read the comment docs.

pidarped commented 3 years ago

The change looks reasonable in that it is good to set the BIndingCallback at InitiateSubscription. SubscribeCancel is not that common from the publisher and, especially since it takes a slightly different path than when the Client terminates the subscription, we need to ensure the Binding is initialized correctly. Approving this to have the changes in. We would need a test case around this path.