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.

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

CoroutineDispatcher.asScheduler for the Rx modules #3150

Closed
wants to merge 10 commits into from

Conversation

dkhalanskyjb
Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb commented Jan 21, 2022

Supersedes #1923
Fixes #968
Fixes #548

Authored by @recheej

@qwwdfsad
Copy link
Member

Could you please squash all of @recheej commits into one?

I find it really hard to navigate, especially now with incorrect GH rendering where all changes seems to be contributed by you:
Screenshot 2022-01-24 at 17 20 01

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
reactive/kotlinx-coroutines-rx2/src/RxScheduler.kt Outdated Show resolved Hide resolved
Copy link
Member

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

Overall looks good

reactive/kotlinx-coroutines-rx2/src/RxScheduler.kt Outdated Show resolved Hide resolved
@dkhalanskyjb
Copy link
Collaborator Author

Kover finds non-covered code in two places:

  • runInterruptible has some uncovered branch;
  • A method called DispatcherScheduler$DispatcherWorker$schedule$1$invoke$$inlined$Runnable$1 was not tested at all, but whatever it is, all lines in the DispatcherWorker class are marked green.

Copy link
Member

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

👍

schedulerJob.cancel()
}

private class DispatcherWorker(val counter: Long, val dispatcher: CoroutineDispatcher, parentJob: Job) : Worker() {
Copy link
Member

Choose a reason for hiding this comment

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

[super optional] typically private can save a getter and a setter

Runnable { scope.launch { task() } }
}

private val workerCounter = atomic(1L)
Copy link
Member

Choose a reason for hiding this comment

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

[code style] it would be nice to both have a line indent here and to keep both properties and functions grouped separately, not mixing them all over the place

recheej and others added 10 commits February 21, 2022 11:33
Also,
* Don't use `kotlinx-coroutines-test`
* Now shutting down the scheduler will interrupt the tasks
The test never did reliably pass, and it doesn't seem like there's
anything in the Scheduler docs that mandates this behavior.
Remove cruft and ensure that they do actually fail if the tasks are
not disposed of.
@dkhalanskyjb
Copy link
Collaborator Author

Merged manually to preserve authorship: d5f852c

@dkhalanskyjb dkhalanskyjb deleted the pull-1923 branch February 21, 2022 09:56
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.

None yet

3 participants