Skip to content

Microsoft: handle sporadic underlying connection was closed, cache webhook_notifications_enabled#458

Merged
squeaky-pl merged 3 commits into
masterfrom
microsoft-underlying-connection-was-closed
Dec 22, 2022
Merged

Microsoft: handle sporadic underlying connection was closed, cache webhook_notifications_enabled#458
squeaky-pl merged 3 commits into
masterfrom
microsoft-underlying-connection-was-closed

Conversation

@squeaky-pl
Copy link
Copy Markdown
Contributor

@squeaky-pl squeaky-pl commented Dec 22, 2022

This takes care of two things:

  • When I changed webhook_notifications_enabled to create a dummy subscription and then delete it I did not realize this is called periodically in a loop. We need to do it only once to know if we are on unlucky account, then we can just cache it.

  • We sporadically get The underlying connection was closed: A connection that was expected to be kept alive was closed by the server.. We get this when subscribing to notifications over webhooks Microsoft makes an initial contact with webhook url. This is safe to retry.

@squeaky-pl squeaky-pl requested a review from drewler December 22, 2022 13:49
@squeaky-pl squeaky-pl merged commit 87e8539 into master Dec 22, 2022
@wojcikstefan wojcikstefan deleted the microsoft-underlying-connection-was-closed branch December 22, 2022 17:00
Comment on lines +221 to +224
# First check if we already have cached value since this function is called
# repeatedly and there is no need to do extra HTTP request every time.
if self._webhook_notifications_enabled is not None:
return self._webhook_notifications_enabled
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it possible that this method is called one time for Account A and another time for Account B? If so, this will have issues. If not, should this method not accept the account argument?

Copy link
Copy Markdown
Contributor Author

@squeaky-pl squeaky-pl Dec 23, 2022

Choose a reason for hiding this comment

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

No. It's always the same account object, I know it's weird given you receive account as parameter but that's how it was done in the past for some reason.

It's:

  • single greenlet that is responsible for account sync
  • that has single events provider for this specific account
  • that has single client for this specific account

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I guess this was done like that with Google because they did not want to make repetitive ORM calls in this component, see Google one.

Copy link
Copy Markdown
Contributor Author

@squeaky-pl squeaky-pl Dec 23, 2022

Choose a reason for hiding this comment

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

But I guess the whole thing could be refactored to accept Account instance in the constructor, but it would be probably a little bit problematic given how session management is handled in sync-engine.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yea, that would make for sense, I think. Should probably note it as an issue or maybe at least leave a TODO comment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See: #459

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.

3 participants