Skip to content

Conversation

@fpillet
Copy link
Member

@fpillet fpillet commented Jan 6, 2017

This PR rewrites both the creation of Action's error observable inside the Action itself and restructures error handling in the execute function.

The motivation is to try and fix #63. The only plausible reason that could explain this issue is that the notEnabledError observable previously created was derived from inputs. Since executionObservable (the root observable for all other work inside action) was derived from inputs too, one couldn't guarantee the order of processing on input update, depending on who subscribed to what and in what order.

To fix the issue, I decided to have a single point of contact with inputs, and derive everything from it. This led to some not-perfectly-Rx-y code where I push errors to an internal subject using side effects, but it really is less important than getting Action to perform right in all circumstances.

I still have to find a way to reproduce issue #63, waiting on @tomburns to confirm whether this change fixes his issues.

We were directly looking at `inputs`, this could cause unpredictable execution order under certain conditions, and trigger a `notEnabledError` before execution begins.
@fpillet
Copy link
Member Author

fpillet commented Jan 6, 2017

Ok so @tomburns confirmed that the fix indeed resolves #63. Looks like we're good to go with this one.

@tomburns
Copy link

tomburns commented Jan 6, 2017

Regarding a repro and possible test case for this:

In our case, we were doing the following, all in the same method:

  1. instantiating the Action
  2. subscribing to elements
  3. subscribing to errors
  4. passing the Action off to its consumer.

@ashfurrow ashfurrow requested a review from ishkawa January 6, 2017 18:08
let subject = ReplaySubject<Element>.createUnbounded()

let work = executionObservables
.map { $0.catchError { throw ActionError.underlyingError($0) } }
Copy link
Member

Choose a reason for hiding this comment

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

Is throw ActionError.underlyingError($0) necessary?
The error seems to appear in errors subject too.

If that is true, catchError { _ in Observable.empty() } is more suitable.

Copy link
Member Author

@fpillet fpillet Jan 8, 2017

Choose a reason for hiding this comment

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

If you look at the previous code, it was acting exactly like my code -- propagate errors to the execution observable returned by the execute() function, both for errors produced by the execution, and for the notEnabled error. I kept the same behaviour.

Copy link
Member

Choose a reason for hiding this comment

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

I got it 👍

return Observable.of(workFactory(input).shareReplay(1))
return Observable.of(workFactory(input)
.do(onError: { errorsSubject.onNext(.underlyingError($0)) })
.shareReplay(1))
Copy link
Member

Choose a reason for hiding this comment

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

nit: Introducing variable for the execution observable makes this part more readable.

let execution = workFactory(input)
    .do(onError: { errorsSubject.onNext(.underlyingError($0)) })
    .shareReplay(1)

return Observable.of(execution)

Copy link
Member Author

@fpillet fpillet Jan 8, 2017

Choose a reason for hiding this comment

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

I don't see how this makes the code more readable. Actually I think it makes it less readable (but that's nitpicking too)

Copy link
Member

Choose a reason for hiding this comment

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

OK. Let's leave it as it is.

@ashfurrow ashfurrow merged commit 24bb86a into RxSwiftCommunity:master Jan 8, 2017
@ashfurrow
Copy link
Member

I'll try to get this released tomorrow, thanks again 👍

@ashfurrow
Copy link
Member

Okay, all released for CocoaPods and Carthage!

@fpillet
Copy link
Member Author

fpillet commented Jan 9, 2017

Thanks, Ash! 👏

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.

Actions defaults to enabled=false

4 participants