search

aws-greengrass / aws-greengrass-shadow-manager

A GreengrassV2 Component that provides offline device shadow documents and optional synchronization to the IoT device shadow service.
Apache License 2.0
9 stars 5 forks source link

fix: handle `AbortedException` correctly when syncing with cloud. #112

Closed nikkhilmuthye closed 2 years ago

nikkhilmuthye commented 2 years ago

Issue #, if available:

Description of changes: If an AbortedException is thrown, check the cause to see if it was caused due to an InterruptedException. If it is, then rethrow that back to the caller. The syncLoop should add this request back in the queue if the sync request failed due to an InterruptedException.

Why is this change necessary: There might be times when a sync request fails due to the thread being interrupted (while syncing) or if the thread just dies.

How was this change tested: Added / Updated unit tests.

Any additional information or context required to review the change:

Checklist:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

github-actions[bot] commented 2 years ago

Unit Tests Coverage Report

File Coverage Lines Branches
All files 87% 91% 82% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.LocalUpdateSyncRequest 84% 88% 80% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.LocalDeleteSyncRequest 99% 98% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.FullShadowSyncRequest 84% 97% 71% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.CloudDeleteSyncRequest 80% 78% 83% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.BaseSyncRequest 78% 55% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.CloudUpdateSyncRequest 74% 76% 71% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.model.Operation 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.model.PubSubRequest 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.strategy.PeriodicSyncStrategy 80% 92% 69% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.strategy.SyncStrategyFactory 100% 100% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.strategy.BaseSyncStrategy 74% 72% 75% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.strategy.RealTimeSyncStrategy 93% 96% 90% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.IotDataPlaneClientFactory 87% 100% 73% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.RequestBlockingQueue 96% 99% 93% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.SyncHandler 76% 92% 60% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.IotDataPlaneClientWrapper 91% 91% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.RequestMerger 74% 71% 77% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.CloudDataClient 75% 76% 75% :white_check_mark:
com.aws.greengrass.shadowmanager.model.configuration.ShadowSyncConfiguration 87% 93% 82% :white_check_mark:
com.aws.greengrass.shadowmanager.model.configuration.ThingShadowSyncConfiguration 76% 78% 75% :white_check_mark:
com.aws.greengrass.shadowmanager.ShadowManagerDatabase 0% 0% 0% :x:
com.aws.greengrass.shadowmanager.ShadowManager 81% 85% 77% :white_check_mark:
com.aws.greengrass.shadowmanager.ShadowManagerDAOImpl 100% 100% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.AuthorizationHandlerWrapper 100% 100% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.ShadowManager$1 50% 50% 0% :x:
software.amazon.awssdk.aws.greengrass.GeneratedAbstractDeleteThingShadowOperationHandler 67% 67% 0% :white_check_mark:
software.amazon.awssdk.aws.greengrass.GeneratedAbstractListNamedShadowsForThingOperationHandler 67% 67% 0% :white_check_mark:
software.amazon.awssdk.aws.greengrass.GeneratedAbstractUpdateThingShadowOperationHandler 67% 67% 0% :white_check_mark:
software.amazon.awssdk.aws.greengrass.GeneratedAbstractGetThingShadowOperationHandler 67% 67% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.strategy.model.StrategyType 86% 92% 80% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.strategy.model.Strategy 90% 100% 80% :white_check_mark:
com.aws.greengrass.shadowmanager.util.JsonMerger 94% 100% 89% :white_check_mark:
com.aws.greengrass.shadowmanager.util.SyncNodeMerger 91% 98% 84% :white_check_mark:
com.aws.greengrass.shadowmanager.util.DataOwner 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.util.ShadowWriteSynchronizeHelper 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.util.Validator 100% 100% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.util.JsonUtil 96% 100% 92% :white_check_mark:
com.aws.greengrass.shadowmanager.model.ResponseMessageBuilder 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.model.ShadowDocument 81% 89% 73% :white_check_mark:
com.aws.greengrass.shadowmanager.model.ShadowStateMetadata 93% 98% 89% :white_check_mark:
com.aws.greengrass.shadowmanager.model.ErrorMessage 75% 100% 50% :white_check_mark:
com.aws.greengrass.shadowmanager.model.ShadowRequest 86% 71% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.model.ShadowState 84% 87% 81% :white_check_mark:
com.aws.greengrass.shadowmanager.model.LogEvents 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.BaseRequestHandler 77% 77% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.GetThingShadowRequestHandler 100% 100% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.PubSubClientWrapper 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.IpcRateLimiter 100% 100% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.UpdateThingShadowIPCHandler 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.InboundRateLimiter 100% 100% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.InboundRateLimiter$1 100% 100% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.DeleteThingShadowRequestHandler 99% 99% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.UpdateThingShadowRequestHandler 91% 95% 88% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.DeleteThingShadowIPCHandler 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.GetThingShadowIPCHandler 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.ListNamedShadowsForThingIPCHandler 100% 100% 100% :white_check_mark:

Minimum allowed coverage is 65%

Generated by :monkey: cobertura-action against 00738cdf7690f94d4beeafbd6997cec59abae63c

github-actions[bot] commented 2 years ago

Integration Tests Coverage Report

File Coverage Lines Branches
All files 62% 68% 55% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.LocalUpdateSyncRequest 34% 47% 20% :x:
com.aws.greengrass.shadowmanager.sync.model.LocalDeleteSyncRequest 55% 59% 50% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.FullShadowSyncRequest 39% 43% 36% :x:
com.aws.greengrass.shadowmanager.sync.model.CloudDeleteSyncRequest 53% 55% 50% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.BaseSyncRequest 78% 55% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.model.CloudUpdateSyncRequest 51% 51% 50% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.model.Operation 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.model.PubSubRequest 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.strategy.PeriodicSyncStrategy 61% 65% 56% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.strategy.SyncStrategyFactory 100% 100% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.strategy.BaseSyncStrategy 75% 75% 75% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.strategy.RealTimeSyncStrategy 74% 69% 80% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.IotDataPlaneClientFactory 85% 100% 69% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.RequestBlockingQueue 53% 62% 43% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.SyncHandler 84% 92% 75% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.IotDataPlaneClientWrapper 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.RequestMerger 19% 29% 10% :x:
com.aws.greengrass.shadowmanager.sync.CloudDataClient 51% 48% 54% :white_check_mark:
com.aws.greengrass.shadowmanager.model.configuration.ShadowSyncConfiguration 70% 79% 61% :white_check_mark:
com.aws.greengrass.shadowmanager.model.configuration.ThingShadowSyncConfiguration 76% 78% 75% :white_check_mark:
com.aws.greengrass.shadowmanager.ShadowManagerDatabase 100% 100% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.ShadowManager 87% 87% 86% :white_check_mark:
com.aws.greengrass.shadowmanager.ShadowManagerDAOImpl 84% 90% 77% :white_check_mark:
com.aws.greengrass.shadowmanager.AuthorizationHandlerWrapper 100% 100% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.ShadowManager$1 17% 17% 0% :x:
software.amazon.awssdk.aws.greengrass.GeneratedAbstractDeleteThingShadowOperationHandler 100% 100% 0% :white_check_mark:
software.amazon.awssdk.aws.greengrass.GeneratedAbstractListNamedShadowsForThingOperationHandler 100% 100% 0% :white_check_mark:
software.amazon.awssdk.aws.greengrass.GeneratedAbstractUpdateThingShadowOperationHandler 100% 100% 0% :white_check_mark:
software.amazon.awssdk.aws.greengrass.GeneratedAbstractGetThingShadowOperationHandler 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.strategy.model.StrategyType 55% 69% 40% :white_check_mark:
com.aws.greengrass.shadowmanager.sync.strategy.model.Strategy 90% 100% 80% :white_check_mark:
com.aws.greengrass.shadowmanager.util.JsonMerger 34% 38% 31% :x:
com.aws.greengrass.shadowmanager.util.SyncNodeMerger 0% 0% 0% :x:
com.aws.greengrass.shadowmanager.util.DataOwner 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.util.ShadowWriteSynchronizeHelper 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.util.Validator 62% 64% 59% :white_check_mark:
com.aws.greengrass.shadowmanager.util.JsonUtil 80% 86% 73% :white_check_mark:
com.aws.greengrass.shadowmanager.model.ResponseMessageBuilder 91% 91% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.model.ShadowDocument 81% 89% 73% :white_check_mark:
com.aws.greengrass.shadowmanager.model.ShadowStateMetadata 81% 86% 77% :white_check_mark:
com.aws.greengrass.shadowmanager.model.ErrorMessage 97% 94% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.model.ShadowRequest 86% 71% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.model.ShadowState 89% 91% 88% :white_check_mark:
com.aws.greengrass.shadowmanager.model.LogEvents 100% 100% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.BaseRequestHandler 77% 77% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.GetThingShadowRequestHandler 87% 74% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.PubSubClientWrapper 80% 80% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.IpcRateLimiter 100% 100% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.UpdateThingShadowIPCHandler 55% 55% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.InboundRateLimiter 94% 100% 88% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.InboundRateLimiter$1 75% 100% 50% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.DeleteThingShadowRequestHandler 86% 72% 100% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.UpdateThingShadowRequestHandler 71% 74% 69% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.DeleteThingShadowIPCHandler 53% 53% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.GetThingShadowIPCHandler 95% 95% 0% :white_check_mark:
com.aws.greengrass.shadowmanager.ipc.ListNamedShadowsForThingIPCHandler 61% 55% 67% :white_check_mark:

Minimum allowed coverage is 45%

Generated by :monkey: cobertura-action against 00738cdf7690f94d4beeafbd6997cec59abae63c

lgtm-com[bot] commented 2 years ago

This pull request introduces 1 alert when merging a6aa760a8cd5624d282876d46a5b845d451e8a07 into 5835e8670c9254c61ebb7cc546ff90d531dc0f75 - view on LGTM.com

new alerts:

rbattle commented 2 years ago

not sure why lgtm thinks the variable could be null - the request is coming from SyncRequest request = syncQueue.take(); and it's not null there.

MikeDombo commented 2 years ago

not sure why lgtm thinks the variable could be null - the request is coming from SyncRequest request = syncQueue.take(); and it's not null there.

take returns the head, if the queue is empty then it would return null most likely. So yes, there should be a null check

nikkhilmuthye commented 2 years ago

not sure why lgtm thinks the variable could be null - the request is coming from SyncRequest request = syncQueue.take(); and it's not null there.

take returns the head, if the queue is empty then it would return null most likely. So yes, there should be a null check

take waits if the queue is empty. If another threads adds an item to the queue, this thread will get signed. So that time the head won't be null.

lgtm-com[bot] commented 2 years ago

This pull request introduces 1 alert when merging 00738cdf7690f94d4beeafbd6997cec59abae63c into 5835e8670c9254c61ebb7cc546ff90d531dc0f75 - view on LGTM.com

new alerts:

rbattle commented 2 years ago

not sure why lgtm thinks the variable could be null - the request is coming from SyncRequest request = syncQueue.take(); and it's not null there.

take returns the head, if the queue is empty then it would return null most likely. So yes, there should be a null check

take waits if the queue is empty. If another threads adds an item to the queue, this thread will get signed. So that time the head won't be null.

I think you meant "this thread will get signaled" - but yes, take waits for an item to be added to the queue.

You also can't add null objects to the queue so it won't be null when take returns.