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

RAC Principles: How best to start SignalProducer only once? #2924

Closed
tomj opened this issue May 22, 2016 · 5 comments
Closed

RAC Principles: How best to start SignalProducer only once? #2924

tomj opened this issue May 22, 2016 · 5 comments
Labels

Comments

@tomj
Copy link
Contributor

tomj commented May 22, 2016

Hi - this came up just now during a discussion with a friend (cc @mgrebenets)

I have a UserManager which retrieves User objects from the network and wish to expose an interface on the UserManager as follows:

func getUsers() -> SignalProducer<User, ErrorType>

(I'm returning a SignalProducer as I want to flatMap this network request with requests to other network resources).

The catch is that I only want to start the network request wrapped up in the SignalProducer if there's not a request currently in progress.

My intuition to solve this went like this:

class UserManager {

    static let sharedManager = UserManager()

    private var requestSignal: Signal<User, NSError>? = nil

    func getUsers() -> SignalProducer<User, NSError> {
        if let requestSignal = requestSignal {
            print("There is already a request in progress so simply return a reference to its signal")
            return SignalProducer(signal: requestSignal)
        } else {
            let requestSignalProducer = SignalProducer<User, NSError> { observer, disposable in
                let url = NSURL(string:"http://jsonplaceholder.typicode.com/users/1")!
                let task = NSURLSession.sharedSession()
                    .dataTaskWithURL(url) { (data, response, error) in
                    if error != nil {
                        observer.sendFailed(NSError(domain:"", code:5, userInfo:nil))
                    } else {
                        let json = try! NSJSONSerialization.JSONObjectWithData(data!, options: NSJSONReadingOptions())
                        let user = User(JSON: json)
                        print("Completed user request at \(NSDate())")
                        observer.sendNext(user)
                        observer.sendCompleted()
                    }
                }
                print("Started user request at \(NSDate())")
                task.resume()
            }
            requestSignalProducer.startWithSignal{ [weak self] (signal, disposable) in
                guard let `self` = self else { return }
                `self`.requestSignal = signal
                signal.observeCompleted {
                    print("Completing the user request signal")
                    `self`.requestSignal = nil
                }
            }
            return SignalProducer(signal: self.requestSignal!)
        }
    }
}

Buuuut the mutable state in the requestSignal feels really "Un-RAC". Would appreciate any pointers you could give on a solution which is more in line with the RAC principles.

Cheers! 😀

@mdiep mdiep added the question label May 22, 2016
@mdiep
Copy link
Contributor

mdiep commented May 22, 2016

Using replayLazily would simplify things a bit—then you could save the SignalProducer directly instead of redirecting a signal.

Also, there's a race condition here, so you might want to use RAC's Atomic type. (If 2 threads call getUsers at the same time, you could could end up with 2 requests.)

I'd also recommend splitting the actual API code into a separate method or class. That will make the code a little easier to read and separate the concerns a little better.

struct API {
    static func getUsers() -> SignalProducer<User, NSError> {
            return SignalProducer<User, NSError> { observer, disposable in
                let url = NSURL(string:"http://jsonplaceholder.typicode.com/users/1")!
                let task = NSURLSession.sharedSession()
                    .dataTaskWithURL(url) { (data, response, error) in
                    if error != nil {
                        observer.sendFailed(NSError(domain:"", code:5, userInfo:nil))
                    } else {
                        let json = try! NSJSONSerialization.JSONObjectWithData(data!, options: NSJSONReadingOptions())
                        let user = User(JSON: json)
                        print("Completed user request at \(NSDate())")
                        observer.sendNext(user)
                        observer.sendCompleted()
                    }
                }
                print("Started user request at \(NSDate())")
                task.resume()
         }
    }
}

class UserManager {
    static let sharedManager = UserManager()

    private var requestProducer: Atomic<SignalProducer<User, NSError>?> = Atomic(nil)

    func getUsers() -> SignalProducer<User, NSError> {
        return requestProducer.modify { producer in
            if let producer = producer {
                print("There is already a request in progress so simply return a reference to its signal")
                return producer
            }

            return API.getUsers()
                .on(completed: {
                    self.requestProcuder.modifify { _ in nil }
                })
               .replayLazily()
        }
    }
}

⚠️ I typed this directly into the browser and it's untested. ⚠️

There still is state, but it's contained a little better.

The RAC-iest way to do it would be to add a new operator that contains the state.

extension SignalProducer {
    func replayIfStarted() -> SignalProducer<Value, Error> {
        let active: Atomic<SignalProducer<Value, Error>?> = nil
        return active.modify { producer in
            if let producer = producer {
                return producer
            }

            return self
                .on(completed: {
                    active.modify { _ in nil }
                })
                .replayLazily()
        }
    }
}

I'd be sure to double check that against the RAC codebase, and also write some tests around the producer lifetime, to make sure I wrote it properly. ☺️

@tomj
Copy link
Contributor Author

tomj commented May 22, 2016

Nice! And apologies for the code-barf on my part, I should have prefaced that block with a "this is not prod code" message 😄

I'll check this out when I'm in front of Xcode; appreciate all your insight.

@mdiep
Copy link
Contributor

mdiep commented May 22, 2016

I'm going to close this. Feel free to reopen if you have questions after looking at it!

@mdiep mdiep closed this as completed May 22, 2016
@tomj
Copy link
Contributor Author

tomj commented May 22, 2016

👍 thanks Matt

@JaviSoto
Copy link
Member

Here's another way to go about this problem: using Property!

final class UserManager {
    /// The public API is as easy as observing this.
    public let users: AnyProperty<[User]>
    private let usersMutableProperty = MutableProperty<[User]>([])

    init() {
        self.users = AnyProperty(self.usersMutableProperty)  
    }

    private func getUsers() -> SignalProducer<User, NSError> { }

    public func requestUsers() {
        /// This is not idea because multiple calls to this method would step onto one-another, just doing it like this for simplicity
        self.usersMutableProperty <~ self.usersRequest()
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants