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.

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

fix: CIRA connection race condition and add 30 sec keepalive time #535

Closed
wants to merge 5 commits into from

Conversation

Ganeshr93
Copy link
Contributor

PR Checklist

  • Unit Tests have been added for new changes
  • API tests have been updated if applicable
  • All commented code has been removed
  • If you've added a dependency, you've ensured license is compatible with Apache 2.0 and clearly outlined the added dependency.

What are you changing?

Fix race condition that happens during a CIRA reconnect
Configure 30 sec keepalive/heartbeat time for all CIRA connections

Anything the reviewer should know when reviewing this PR?

If the there are associated PRs in other repositories, please link them here (i.e. open-amt-cloud-toolkit/repo#365 )

@@ -177,7 +184,9 @@ export class MPSServer {
onClose = async (socket: CIRASocket): Promise<void> => {
logger.debug('MPS:CIRA connection closed')
try {
await this.handleDeviceDisconnect(socket.tag.nodeid)
if (devices[socket.tag.nodeid]?.ciraSocket?.tag.id === socket?.tag.id) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should log on the else condition to know when this is happening?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it will be good to have that. Should I do a separate PR for adding/modifying logs around CIRA connections (especially around all socket events)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would do it as part of this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added extra logging around cira connection events with the new commit, let me know your feedback on the changes

@codecov
Copy link

codecov bot commented Jan 31, 2022

Codecov Report

Merging #535 (8d1d26f) into main (8f7f628) will decrease coverage by 4.35%.
The diff coverage is 41.66%.

❗ Current head 8d1d26f differs from pull request most recent head 8645121. Consider uploading reports for the commit 8645121 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #535      +/-   ##
==========================================
- Coverage   91.24%   86.88%   -4.36%     
==========================================
  Files          68       66       -2     
  Lines        2959     2837     -122     
  Branches      449      475      +26     
==========================================
- Hits         2700     2465     -235     
- Misses        259      355      +96     
- Partials        0       17      +17     
Impacted Files Coverage Δ
src/models/models.ts 100.00% <ø> (ø)
src/server/mpsserver.ts 9.02% <4.54%> (-84.43%) ⬇️
src/amt/APFProcessor.ts 99.16% <100.00%> (+0.03%) ⬆️
src/index.ts 0.00% <0.00%> (ø)
src/amt/DeviceAction.ts 90.75% <0.00%> (ø)
src/utils/wsRedirect.ts 79.62% <0.00%> (ø)
src/routes/auth/login.ts 100.00% <0.00%> (ø)
src/utils/MqttProvider.ts 94.44% <0.00%> (ø)
src/utils/certificates.ts 100.00% <0.00%> (ø)
... and 13 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 8f7f628...8645121. Read the comment docs.

Copy link
Member

@rsdmike rsdmike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you need to add some missing unit tests

@Ganeshr93
Copy link
Contributor Author

Looks like you need to add some missing unit tests
Done. Added unit test for APFProcessor

rsdmike
rsdmike previously approved these changes Feb 2, 2022
@rsdmike rsdmike linked an issue Feb 3, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CIRA connection getting dropped randomly
3 participants