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.

Sign up for GitHub

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

Jump to bottom

Add way to convert CoroutineDispatcher to Scheduler #1923

Closed
wants to merge 76 commits into from
Closed

Add way to convert CoroutineDispatcher to Scheduler #1923

wants to merge 76 commits into from

Conversation

recheej
Copy link
Contributor

@recheej recheej commented Apr 19, 2020 •
edited

Fixes #968 and fixes #548 (duplicates).

@recheej recheej changed the title Rejozil/dispatchertoscheduler Add way to convert CoroutineDispatcher to Scheduler Apr 19, 2020
elizarov
elizarov suggested changes Apr 20, 2020
View reviewed changes
reactive/kotlinx-coroutines-rx2/src/RxScheduler.kt Outdated Show resolved Hide resolved
reactive/kotlinx-coroutines-rx2/src/RxScheduler.kt Outdated Show resolved Hide resolved
reactive/kotlinx-coroutines-rx2/test/SchedulerTest.kt Outdated Show resolved Hide resolved
reactive/kotlinx-coroutines-rx2/src/RxScheduler.kt Outdated Show resolved Hide resolved
reactive/kotlinx-coroutines-rx2/src/RxScheduler.kt Outdated Show resolved Hide resolved
reactive/kotlinx-coroutines-rx2/src/RxScheduler.kt Outdated Show resolved Hide resolved
@recheej
Copy link
Contributor Author

recheej commented Apr 21, 2020

@elizarov ready for review again. Indeed there were some OOM issues so I simplified the job handling. as per your recommendations.

@recheej recheej requested a review from elizarov April 21, 2020 05:26
@recheej recheej requested a review from elizarov April 21, 2020 16:29
@recheej recheej marked this pull request as draft April 24, 2020 04:35
@recheej recheej marked this pull request as ready for review April 26, 2020 20:10
@recheej recheej requested a review from elizarov April 26, 2020 20:10
@recheej recheej force-pushed the rejozil/dispatchertoscheduler branch from 22351e6 to 556cf91 Compare April 26, 2020 20:11
@recheej
Copy link
Contributor Author

recheej commented Apr 26, 2020

@elizarov I created new API: Job.asDisposable(). What steps should I follow to make sure the api is documented, published, etc?

@recheej recheej force-pushed the rejozil/dispatchertoscheduler branch from 556cf91 to 2d89cbf Compare April 26, 2020 20:23
zach-klippenstein
zach-klippenstein reviewed Apr 27, 2020
View reviewed changes
reactive/kotlinx-coroutines-rx2/src/RxScheduler.kt Outdated Show resolved Hide resolved
reactive/kotlinx-coroutines-rx2/src/RxScheduler.kt Show resolved Hide resolved
reactive/kotlinx-coroutines-rx2/src/RxScheduler.kt Outdated Show resolved Hide resolved
reactive/kotlinx-coroutines-rx2/src/RxScheduler.kt Outdated Show resolved Hide resolved
reactive/kotlinx-coroutines-rx2/src/RxScheduler.kt Outdated Show resolved Hide resolved
reactive/kotlinx-coroutines-rx2/src/RxScheduler.kt Outdated Show resolved Hide resolved
reactive/kotlinx-coroutines-rx2/src/RxScheduler.kt Outdated Show resolved Hide resolved
reactive/kotlinx-coroutines-rx2/src/RxScheduler.kt Outdated Show resolved Hide resolved
reactive/kotlinx-coroutines-rx2/src/RxScheduler.kt Outdated Show resolved Hide resolved
@recheej recheej force-pushed the rejozil/dispatchertoscheduler branch from 32d51ae to 3777437 Compare April 29, 2020 01:13
@recheej
Copy link
Contributor Author

recheej commented Apr 29, 2020

@elizarov ready for review again.

@recheej
Copy link
Contributor Author

recheej commented Apr 30, 2020

Before this gets merged I will squash + rebase

zach-klippenstein
zach-klippenstein suggested changes Apr 30, 2020
View reviewed changes
Copy link
Contributor

@zach-klippenstein zach-klippenstein 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 the last update accidentally broke some scheduling logic, and there are still more tests that need to be written.

One general bit of feedback on these tests is that they shouldn't actually cause any real delays. Unit tests should really not actually sleep at all if possible - they should be very fast, and every additional sleep slows down every future PR. The coroutine library has great support for testing with special dispatchers that give you fine-grained control over time, and those tools would be very useful for testing this very time-dependent logic. Here are some links to look at:

reactive/kotlinx-coroutines-rx2/src/RxScheduler.kt Outdated Show resolved Hide resolved
reactive/kotlinx-coroutines-rx2/src/RxScheduler.kt Outdated Show resolved Hide resolved
reactive/kotlinx-coroutines-rx2/src/RxScheduler.kt Show resolved Hide resolved
reactive/kotlinx-coroutines-rx2/src/RxScheduler.kt Outdated Show resolved Hide resolved
reactive/kotlinx-coroutines-rx2/test/SchedulerStressTest.kt Outdated Show resolved Hide resolved
reactive/kotlinx-coroutines-rx2/test/SchedulerTest.kt Show resolved Hide resolved
reactive/kotlinx-coroutines-rx2/test/SchedulerTest.kt Outdated Show resolved Hide resolved
reactive/kotlinx-coroutines-rx2/test/SchedulerTest.kt Outdated Show resolved Hide resolved
reactive/kotlinx-coroutines-rx2/test/SchedulerTest.kt Show resolved Hide resolved
reactive/kotlinx-coroutines-rx2/test/SchedulerTest.kt Outdated Show resolved Hide resolved
@recheej
Copy link
Contributor Author

recheej commented May 5, 2020

@zach-klippenstein @elizarov ready for review.

@recheej recheej force-pushed the rejozil/dispatchertoscheduler branch 4 times, most recently from b8f996f to 91da78f Compare May 7, 2020 01:54
@elizarov
Copy link
Contributor

elizarov commented May 7, 2020

Please, ping me when you're done and its ready for review.

@recheej
Copy link
Contributor Author

recheej commented May 7, 2020

@elizarov this is ready for review

@recheej
Copy link
Contributor Author

recheej commented May 10, 2020

@zach-klippenstein @elizarov ready for review

zach-klippenstein
zach-klippenstein suggested changes May 11, 2020
View reviewed changes
Copy link
Contributor

@zach-klippenstein zach-klippenstein left a comment

Choose a reason for hiding this comment

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

The implementation mostly looks good, just two things left. I haven't re-reviewed the tests, I'm going to leave those for Roman since he's much more experienced writing tests for this stuff.

reactive/kotlinx-coroutines-rx2/src/RxScheduler.kt Outdated Show resolved Hide resolved
reactive/kotlinx-coroutines-rx2/src/RxScheduler.kt Outdated Show resolved Hide resolved
Rechee and others added 21 commits December 28, 2020 12:33
@recheej
make worker job private
a100fb0
@recheej
check for job cancelled
554ef38
@recheej
remove println
33d5c67
@recheej
refactor tests to be more generic for worker tests in next commit
9e100c2
@recheej
wrote tests for scheduler worker and made fixes based off tests
446c787
@recheej
updating scheduler stress tests to include worker tests
631110b
@recheej
make sure we're waiting for delayJob properly
e0d70d4
@recheej
refactor delay logic (again :)) to delay first before enqueuing work
cb506a6
@recheej
remove redundant expect
0477dfd
@recheej
remove redundant channel close
53e0122
@recheej
using one job for both non-delayed and delayed. also fix redundant jo…
7b361b3 
…b with task itself
@recheej
remove redundant modifier
26c9e1e
@recheej
api dump
e5e86f1
@recheej
use start function and reuse JobDisposable
64b3743
@recheej
remove addTaskToQueue function
78457b3
@recheej
fix typo in tests
5f5e637
@recheej
unit test for multiple workers
b634a8c
@recheej
maintain binary compat. also formatting
8f204bf
@recheej
api dump
a829462
@recheej
change if statements
15f96b3
@recheej
add asScheduler extension for rx3
2aab409
@recheej recheej force-pushed the rejozil/dispatchertoscheduler branch from d474b59 to 2aab409 Compare December 28, 2020 21:48
@recheej
Copy link
Contributor Author

recheej commented Dec 28, 2020

@elizarov I went ahead and copied the changes over for rx3. let me know what else you think can be done.

@dkhalanskyjb dkhalanskyjb mentioned this pull request Apr 2, 2021
Closed
@denis-bezrukov
Copy link

Is there a plan to merge this? it seems this pr was ready to merge almost year ago
Would be great to see it as a part of 1.6 😊

@dkhalanskyjb dkhalanskyjb self-requested a review January 18, 2022 13:15
@dkhalanskyjb dkhalanskyjb mentioned this pull request Jan 21, 2022
Closed
@dkhalanskyjb
Copy link
Collaborator

Thanks! We will edit this a bit in #3150 before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zach-klippenstein zach-klippenstein zach-klippenstein approved these changes

@elizarov elizarov elizarov approved these changes

@SuperCatDev SuperCatDev SuperCatDev approved these changes

@dkhalanskyjb dkhalanskyjb Awaiting requested review from dkhalanskyjb

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@recheej @elizarov @zach-klippenstein @denis-bezrukov @dkhalanskyjb @SuperCatDev