Fixing potential issue with RxSwift request method

[magi82]

Before anything else, thank you for this library. Recently, I attempted to network with the server using the request method of RxSwift Unfortunately, I received no response.

MoyaProvider<API>().rx.request(.version)
    .debug()
    .subscribe {
        print($0)
    }
    .disposed(by: self.disposeBag)

2017-09-19 16:34:12.833: SplashViewModel.swift:97 (setVersionInfo()) -> subscribed

I assume it was due to the fact that Single<Response has been captured as weak within the RxProvider’s create closure. I propose that it is better to have a potential retain cycle rather than having no response at all. Therefore, I’ve tried to circumvent the issue by having it similar to that of the requestWithProgress method which retains. Again, I appreciate your service for the community.

MoyaProvider<API>().rx.request(.version)
    .debug()
    .subscribe {
        print($0)
    }
    .disposed(by: self.disposeBag)

2017-09-19 16:36:50.421: SplashViewModel.swift:97 (setVersionInfo()) -> subscribed
2017-09-19 16:36:50.637: SplashViewModel.swift:97 (setVersionInfo()) -> Event next(Status Code: 200, Data Length: 281)
success(Status Code: 200, Data Length: 281)
2017-09-19 16:36:50.640: SplashViewModel.swift:97 (setVersionInfo()) -> Event completed
2017-09-19 16:36:50.640: SplashViewModel.swift:97 (setVersionInfo()) -> isDisposed

[MoyaBot]

	2 Warnings
⚠️	Any changes to library code should be reflected in the Changelog. Please consider adding a note there.
⚠️	The library files were changed, but the tests remained unmodified. Consider updating or adding to the tests to match the library changes.

SwiftLint found issues

Warnings

File	Line	Reason
MoyaProvider+Rx.swift	39	Lines should not have trailing whitespace.
MoyaProvider+Rx.swift	44	Lines should not have trailing whitespace.

Generated by 🚫 Danger

[codecov-io]

Codecov Report

Merging #1294 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1294      +/-   ##
==========================================
+ Coverage      83%   83.04%   +0.04%     
==========================================
  Files          24       24              
  Lines         753      755       +2     
==========================================
+ Hits          625      627       +2     
  Misses        128      128

Impacted Files	Coverage Δ
Sources/RxMoya/MoyaProvider+Rx.swift	92.1% <100%> (+0.43%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1eb1810...e51e71a. Read the comment docs.

[AndrewSB]

Hmm, I have a feeling you should be storing a strong reference to your provider somewhere instead. This reminds me of #1267, can you try that and see if it fixes your issue?

[volodg]

@AndrewSB

1. Created Observable/Single should guarantee callback calling
2. Before request was storing reference to provider, so this changes incompatible with legacy code
3. rxRequestWithProgress is holding the strong reference, why it is ok for this method and bad for similar one: request, be consistent.
4. It is normal when closure holds required info for call
5. In my source code, I am creating separate provider with different parameters for each request, so it will be quite complicated task to write holding logic for such scenario to avoid leaks

[AndrewSB]

I'm reading through RxSwift's guide to disposing and, in my potentially uninformed mindset of things Rx, it seems to me like you have to hold onto your provider if you want your subscriptions to stay alive.

Can we arrive at a consensus for that? @sunshinejr @pedrovereza @freak4pc what do you think?

Also, if that is the case, maybe we shouldn't be capturing self in rxRequestWithProgress

[sunshinejr]

I believe we always guided to retain the provider to get the correct behavior. We will need to investigate why there is no response for a Single extension, though - there should be at least an error. Also we should check the consistency across both RxSwift request methods as well as both ReactiveSwift methods.

[volodg]

@iwheelbuy thanks a lot, I did not know about it before, but nevertheless, even if a request was released there should be at least an error.

[AndrewSB]

even if a request was released there should be at least an error.

@volodg I disagree, I think Rx's philosophy is to do nothing when an Observable is disposed, the pattern doesn't recommend sending an error event before disposing.
If you don't want to retain the provider, then you need to handle the disposed case just like the error case

[volodg]

@AndrewSB how to handle the disposed case when no callback is called?

[sunshinejr]

@AndrewSB I agree in the terms of Rx, but the Single type is either a value or a failure, so if there is no value in it we can't really complete because there is no such event for this trait. From docs:

A Single is a variation of Observable that, instead of emitting a series of elements, is always guaranteed to emit either a single element or an error.

This makes me think that implementing an error there might be the right thing to do to not break the contract. What do you think?

[AndrewSB]

I think we should do whatever RxSwift does whenever a Single created through someObservable.asSingle() is disposed.
If that Single receives an error on disposal, lets do the same, else dispose without erring.

@volodg do you want to try to see what happens and post your findings?

[AndrewSB]

@sunshinejr: this may be a good candidate for hacktoberfest!

[volodg]

@AndrewSB
this example:

    let singleStream = Observable<Int>
      .interval(1, scheduler: MainScheduler.instance)
      .timeout(1, scheduler: MainScheduler.instance)
      .asSingle()
      .do(onNext: { _ in
      print("onNext")
    }, onError: { _ in
      print("onError")
    }, onSubscribe: {
      print("onSubscribe")
    }, onSubscribed: {
      print("onSubscribed")
    }, onDispose: {
      print("onDispose")
    })
    
    singleStream.subscribe().dispose()

prints:

onSubscribe
onSubscribed
onDispose

[AndrewSB]

#1311 does The Right Thing and supersedes this