Closed Bug 652221 Opened 10 years ago Closed 10 years ago

War on Spinning the Event Loop: use AsyncResource

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

Attachments

(2 files, 3 obsolete files)

The first of the work item bugs for Bug 600059.

Let's start by eliminating our use of Resource non-invasively, moving functionality into *Cb versions of functions until we can bootstrap async higher and higher up the stack.
Attached file Part 0: minor test tweaks. (obsolete) —
Attachment #535510 - Attachment description: Pointer to pull request → Part 0: minor test tweaks.
Attachment #535510 - Attachment is obsolete: true
Attached file Pointer to pull request (obsolete) —
Sorry for the bugspam, folks; bzpatch's argument order is wrong in the helpstring.
Summary: War on Sync: use AsyncResource → War on Spinning the Event Loop: use AsyncResource
Attached file Pull request: parts 0 through 4. (obsolete) —
Attachment #535511 - Attachment is obsolete: true
Comment on attachment 535514 [details]
Pull request: parts 0 through 4.

Part 0: Convert test_corrupt_keys.js to add_test/run_next_test.
Part 1: Add a test for URI construction and query modification.
Part 2: Add Utils.slices.
Part 3: Move async utilities into async.js. Add Async.spinningly as a temporary shim for wrapping Cb versions of functions.
Part 4: Move Resource.serverTime to AsyncResource, fix comments for AsyncResource.
Attachment #535514 - Attachment description: Pointer to pull request → Pull request: parts 0 through 4.
Attachment #535514 - Flags: review?(philipp)
Addressed comments: 

* Re-split commits to address some cross-mojination
* Added URI tests to test_resource_async.js
* Renamed Async.spinningly to Async.makeSpinningCallback
* Misc comment improvements
* Make Utils.slices and Async.barrieredCallbacks into generators.

Working on one test hang before each individual commit passes all tests, as well as the whole set of commits, but all of the reviewed code so far is good to go.

Philipp, I will submit a fresh pull request for the parts you've reviewed and I've amended.
Attached file Parts 0 through 4, v2
Attachment #535618 - Attachment description: Pointer to pull request → Parts 0 through 4, v2
Attachment #535618 - Flags: review?(philipp)
Attachment #535514 - Attachment is obsolete: true
Attachment #535514 - Flags: review?(philipp)
Attachment #535618 - Flags: review?(philipp) → review+
(In reply to comment #8)
> Batch one:

Thanks. A merge commit mentioning the bug number would've been good (that's the only one that would've needed the r=philikon bit, too.
(In reply to comment #9)
 
> Thanks. A merge commit mentioning the bug number would've been good (that's
> the only one that would've needed the r=philikon bit, too.

Yeah, I considered that, but thought that this way would make for easier rebasing of part two, and a more granular history besides. I plan on using a merge commit for the inextricably linked final parts, but these stand alone.

My bad for omitting the bug number! Was kinda distracted by hg-git woes.
Gah, realized the reason why I'm still waiting for review for the rest is because I never formally asked for it.

Will fire up another push request.
Assignee: rnewman → nobody
Attached file Parts 5 through 11, v1
I don't expect all of these to get through without changes, but best to start somewhere!

All 101 tests pass with these changes.
Assignee: nobody → rnewman
Attachment #538809 - Flags: review?(philipp)
We've abandoned this work in favour of a better approach (repositories, synchronizer, etc.)
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
Comment on attachment 538809 [details]
Parts 5 through 11, v1

Clearing review flag on WONTFIXed bug.
Attachment #538809 - Flags: review?(philipp)
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.