Issuing RPC calls from multiple background threads clobbers the requestId

[eliburke]

I have a scenario where I must issue multiple RPCs via swamp and then wait for all of them to complete before taking another step. I solved my use case with a DispatchGroup in manual mode (calling DispatchGroup.join() before issuing swampSession.call, calling DispatchGroup.leave() in the closures, and then using DispatchGroup.notify to wait for all issued calls to finish.

This mostly works fine when the DispatchGroup is managed from the main thread, or larded up with debug statements. But occasionally it hangs; not all of the RPC calls succeed so the group is never notified.

I tracked the problem down to the requestIds of the various RPC calls. Because swift variables are not atomic, the default implementation here is not thread safe. Multiple threads can increment the counter and then overwrite the result.

    fileprivate func generateRequestId() -> Int {
        self.currRequestId += 1
        return self.currRequestId
    }

This tweaked version uses an unfair lock to make the increment thread safe. It's pretty much the best option for Swift 3. Note that the stack variable is necessary otherwise another thread could increment the requestId after the lock is released, before it is returned.

    private var unfair_lock = os_unfair_lock_s()
    fileprivate func generateRequestId() -> Int {
        os_unfair_lock_lock(&unfair_lock)
        let newRequestId = self.currRequestId + 1
        self.currRequestId = newRequestId
        os_unfair_lock_unlock(&unfair_lock)
        return newRequestId
    }

I am not certain that there are no other threading issues, but this appears to have fixed my one issue nicely.

[eliburke]

As it turns out, the problem is bigger than I thought last night. The callback arrays (callRequests, subscribeRequests, unsubscribeRequests) also need to be made thread safe. Apparently code like this: self.callRequests[callRequestId] = (callback: onSuccess, errorCallback: onError ) can fail because of how Swift's arrays are mutated.

Since generateRequestId() is only available internally, I think a better solution would be to guard each of call(), subscribe(), unsubscribe(), publish() with either an unfair lock or with a serial DispatchQueue. I think the queue is probably a better choice because then there is no chance of blocking the UI thread.

I'm going to make this change locally. If you are interested l can submit a PR.