Bug 1687491 - Dispatch all external API calls #40
Conversation
This file contains 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
added 8 commits
February 4, 2021 12:02
This commit makes multiple tests fail, that is because by dispatching tasks we cannot await on them anymore and this makes tests assertion run before tasks are completed.
Initially I was simply going to use one specific rule from this set of recommended rules "require-await", which throws a linter error when you `await` on an expression which does not return a promise. That was going to help me remove the `await`s from all the functions that no longer return promises after the dispatcher changes. It's importat to make this a rule, because during the dispatcher devolpment I have run into many hard to find bugs related to awaiting on sync stuff. Anyways, it turned out this small set of linter rules flagged a bunch of bugs which we were not aware, thus I left the whole set there.
...and `testLaunch` all `testGetValue` functions. I considered simply using testBlockOnQueue for waiting on tasks. But that has two main issues: - It may hang for a long time if tasks keep being enqueued; - It does not guarantee that we continue on the expected moment. For example, imagine we launch multiple tasks for setting the same string. When we await on `testBlockOnQueue` we will wait for all the setting tasks and not necessarily on the one we are expecting. This will result in assertions at the unexpected time.
It was inevitable to provide a way to execute recording for specific metrics without disptaching. Let's take for example `ping.submit`. That is a dispatched task. When we submit a ping we always record two metrics: the sequence number and the start_time. If by any chance we lauched three ping submissions in a row the order the dispatcher might execute the tasks is: - ping.submit - ping.submit - ping.submit - seq.add - start_time.set - seq.add - start_time.set - seq.add - start_time.set This would mean that all pings would be sent with the same sequence number and start_time, since the recordings would only happen after all pings were submitted. Intuitively, it seems that all metric recording inside of Glean.js itself should be done synchronously. I opened Bug 1691037 to verify this assumption and write documentation about this.
brizental
commented
Feb 5, 2021
Dexterp37
requested changes
Feb 5, 2021
brizental
commented
Feb 5, 2021
brizental
commented
Feb 5, 2021
Co-authored-by: Alessio Placitelli <alessio.placitelli@gmail.com>
Dexterp37
approved these changes
Feb 8, 2021
| }) | ||
| .catch(error => { | ||
| console.error( | ||
| "IMPOSSIBLE: Something went wrong while the dispatcher was executing the tasks queue.", |
There was a problem hiding this comment.
Bets on when we'll see this message?
There was a problem hiding this comment.
no bets, unless we change the executeTask to not catch errors or use the observers for something else. right now it should be impossible for this to error. i only added the catch here because the linter was complaining :D
| @@ -112,7 +112,7 @@ describe("PingUploader", function() { | |||
|
|
|||
| // Trigger uploading, but don't wait for it to finish, | |||
| // so that it is ongoing when we cancel. | |||
| uploader.triggerUpload(); | |||
| void uploader.triggerUpload(); | |||
There was a problem hiding this comment.
What's this void here for?
There was a problem hiding this comment.
ah, this is also a complaint from the linter, about not doing anything with a statement that returns a promise. but the void is pretty cool. it is not a typescript operator, it is a javacript operator and what it does is evalute whatever expression comes after it and return undefined.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Not only does this change the code to dispatch all external API calls but also it adequates the dispatcher and other parts of the code for all to work as expected.
I attempted to separate the changes as well as possible in specific commits and be very descriptive in my commit messages. Sincerely hope that makes review clearer.
It seems this could have been broken in multiple PRs, but either tests would fail or the change wouldn't make sense by itself.