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

Fix missing scheduler on Linux and Windows (JVM) #1503

Merged
merged 3 commits into from
Sep 7, 2023

Conversation

cmelchior
Copy link
Contributor

@cmelchior cmelchior commented Sep 6, 2023

Closes #1502

This PR aligns the behavior of schedulers across all platforms. Previously Darwin was using their custom run-loop, which was why it worked there while Linux and Windows failed.

The fix has been verified on Github Actions: https://github.com/realm/realm-kotlin/actions/runs/6098168339

Copy link
Contributor

@rorbech rorbech left a comment

Choose a reason for hiding this comment

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

Don't want to block the release, but it seems to me that the scheduler C-API should be reworked to enable us to generate the generic scheduler and not the default that could fail. Maybe file an issue for that.

Assuming that you actually verified this on the GHA branch?


realm_scheduler_t*
realm_create_generic_scheduler() {
return new realm_scheduler_t{realm::util::Scheduler::make_generic()};
Copy link
Contributor

@rorbech rorbech Sep 6, 2023

Choose a reason for hiding this comment

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

Hmm. Seems weird that the C-API offers an API around default schedulers that aren't necessarily supported and does not have a API for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, it feels weird. I'll file an issue in Core for it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider using DummyScheduler (via Scheduler::make_dummy()) its implementation is simpler & platform agnostic (is_on_thread doesn't invoke std::this_thread::get_id)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But that one doesn't do anything, correct? Which seems to make it dangerous to use unless specifically requested?

Copy link
Collaborator

@nhachicha nhachicha Sep 6, 2023

Choose a reason for hiding this comment

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

That's the goal, the generic one also don't do anything. They are mainly used for use case where we want to open a Realm without notifications (like the Frozen Realms or when doing a compact). The dummy scheduler's implementation is slightly simpler that's all (both don't deliver anything)

https://github.com/realm/realm-core/blob/8ad69b511e2e6e17ee2b2d23a392d02a72c9b227/src/realm/object-store/util/scheduler.cpp#L72-L88

https://github.com/realm/realm-core/blob/8ad69b511e2e6e17ee2b2d23a392d02a72c9b227/src/realm/object-store/util/generic/scheduler.hpp#L24-L46

Copy link
Contributor

@clementetb clementetb left a comment

Choose a reason for hiding this comment

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

Happy once CI is happy.


realm_scheduler_t*
realm_create_generic_scheduler() {
return new realm_scheduler_t{realm::util::Scheduler::make_generic()};
Copy link
Collaborator

Choose a reason for hiding this comment

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

formatting


realm_scheduler_t*
realm_create_generic_scheduler() {
return new realm_scheduler_t{realm::util::Scheduler::make_generic()};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider using DummyScheduler (via Scheduler::make_dummy()) its implementation is simpler & platform agnostic (is_on_thread doesn't invoke std::this_thread::get_id)

@cmelchior cmelchior merged commit 3747d21 into releases Sep 7, 2023
1 check passed
@cmelchior cmelchior deleted the cm/fix-default-scheduler branch September 7, 2023 07:29
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants