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

SubscribeOn/ObserveOn Implementation #199

Closed
wants to merge 13 commits into from

Conversation

mairbek
Copy link
Contributor

@mairbek mairbek commented Mar 19, 2013

Implementation SubscribeOn #11 and ObserveOn #12 alongside with basic Schedulers implementation #19.

@cloudbees-pull-request-builder

RxJava-pull-requests #39 FAILURE
Looks like there's a problem with this pull request

@mairbek
Copy link
Contributor Author

mairbek commented Mar 19, 2013

.NET implementation of a Scheduler interface contains methods like

Schedule<TState>(TState, Func<IScheduler, TState, IDisposable>) 

To be honest, I don't quite understand why it is designed to use Func<IScheduler, TState, IDisposable> function as an action. I used Func0<Subscription> in the implementation and it seems to fit well.

@cloudbees-pull-request-builder

RxJava-pull-requests #40 FAILURE
Looks like there's a problem with this pull request

@benjchristensen
Copy link
Member

This is great stuff @mairbek, thank you!

This one is going to result in a lot of discussion so won't be pulled in right away and will likely involve some changes.

For example, a discussion is being kicked off with Erik Meijer (inventor of Rx at Microsoft) to clarify some design decisions on this.

@benjchristensen
Copy link
Member

@sgudiboina if you can join in on the review of this it would be helpful.

@mairbek Erik himself has stated on Twitter that RxJava needs Schedulers so this is definitely high priority!

Anyone else with time to get involved please do. The design of Schedulers will have long-lasting impact so we want to get it as close to right as possible now.

@benjchristensen
Copy link
Member

Anyone getting involved please review #19 for context.


import static org.mockito.Mockito.*;

public class CurrentThreadScheduler extends AbstractScheduler {
Copy link
Member

Choose a reason for hiding this comment

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

This took me a bit to grok but I think I got it and I believe it matches how Rx describes it:

The CurrentThreadScheduler (by accessing the static CurrentThread property) will schedule actions to be performed on the thread that makes the original call. The action is not executed immediately, but is placed in a queue and only executed after the current action is complete.

http://msdn.microsoft.com/en-us/library/hh242963(v=vs.103).aspx

This example also helped me: http://www.introtorx.com/content/v1.0.10621.0/15_SchedulingAndThreading.html#Current

It looks like you might have gotten the unit test from there ... so I think this behaves correctly if I'm understanding it right.

From what I can tell this scheduler is useful when doing nested calls (like the repeat operator) to be a "trampoline" and allow recursion without overflowing the stack ... correct?

@benjchristensen
Copy link
Member

@mairbek This is great work, thank you for the research and thought you obviously did for this and the clean and well-written code.

I've posted my questions and comments via the inline code mechanism so they are contextual.

@cloudbees-pull-request-builder

RxJava-pull-requests #71 FAILURE
Looks like there's a problem with this pull request

@cloudbees-pull-request-builder

RxJava-pull-requests #74 FAILURE
Looks like there's a problem with this pull request

@benjchristensen
Copy link
Member

This request can no longer be automatically merged so I have manually merged it and done some further work on top of your commits at #225.

Please review what I've done. If possible I'd like to merge and release within the next 24 hours.

I'm closing this request since all of your commits are now contained on #225.

Great work @mairbek I appreciate you taking this on.

benjchristensen added a commit that referenced this pull request Apr 5, 2013
rickbw pushed a commit to rickbw/RxJava that referenced this pull request Jan 9, 2014
jihoonson pushed a commit to jihoonson/RxJava that referenced this pull request Mar 6, 2020
Initial suggestion for a feign <-> resilience4j module
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.

3 participants