fix(notifications): extension activation hangs if fetch fails #6052
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as resolved.
This comment was marked as resolved.
hayemaxi
force-pushed
the
notifications6
branch
3 times, most recently
from
3e4a5d6 to
903c23c
Compare
Problem: If notifications fail to fetch, it will retry multiple times with 30 second waits. Activation is awaiting on this though, so it may take forever to finish. Solution: don't await on notification activation Also, update logging statements.
hayemaxi
force-pushed
the
notifications6
branch
from
903c23c to
4ea6d98
Compare
jpinkney-aws
approved these changes
Nov 20, 2024
- Simplify when to focus the notifications panel for emergency notifications. - Try to limit race conditions when dismissing notifications.
hayemaxi
force-pushed
the
notifications6
branch
from
8485691 to
540ce54
Compare
justinmk3
reviewed
Nov 25, 2024
| getLogger('notifications').debug('Activated in-IDE notifications polling module') | ||
| logger.debug('Activated in-IDE notifications polling module') | ||
| } catch (err) { | ||
| logger.error('Failed to activate in-IDE notifications module.') |
Contributor
There was a problem hiding this comment.
.... %O', err)`
or err.message
justinmk3
reviewed
Nov 25, 2024
| }) | ||
|
|
||
| await activateNotifications(context, authState, getAuthState) | ||
| void activateNotifications(context, authState, getAuthState) |
Contributor
There was a problem hiding this comment.
if possible it's helpful to make activateNotications non-async, so that it clearly signals that it does its own logging and .catch() of async functions itself.
justinmk3
reviewed
Nov 25, 2024
Comment on lines
+279
to
+285
| return withRetries( | ||
| async () => { | ||
| try { | ||
| return await fetcher.getNewETagContent(versionTag) | ||
| } catch (err) { | ||
| logger.error('Failed to fetch at endpoint: %s, err: %s', endpoint, err) | ||
| throw err |
Contributor
There was a problem hiding this comment.
withRetries should do useful logging of failure by default. We have so much nesting of these wrapper functions, we might as well do something with it...
Contributor
Author
|
Will follow up with your comments @justinmk3 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
If notifications fail to fetch, it will retry multiple times with 30 second waits. Activation is awaiting on this though, so it may take forever to finish.
Solution
don't await on notification activation
Also, update logging statements.
License: I confirm that my contribution is made under the terms of the Apache 2.0 license.