[Accepted with Revisions] SDL 0112 - SDL iOS Move RPC Notifications to a Serial Background Queue

[theresalech]

Hello SDL community,

The review of "SDL iOS Move RPC Notifications to a Concurrent Background Queue" begins now and runs through November 21, 2017. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0112-ios-serial-rpc-notifications.md

Reviews are an important part of the SDL evolution process. All reviews should be sent to the associated Github issue at:

https://github.com/smartdevicelink/sdl_evolution/issues/346

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of SDL. When writing your review, here are some questions you might want to answer in your review:

• Is the problem being addressed significant enough to warrant a change to SDL?
• Does this proposal fit well with the feel and direction of SDL?
• If you have used competitors with a similar feature, how do you feel that this proposal compares to those?
• How much effort did you put into your review? A glance, a quick reading, or an in-depth study? Please state explicitly whether you believe that the proposal should be accepted into SDL.

More information about the SDL evolution process is available at

https://github.com/smartdevicelink/sdl_evolution/blob/master/process.md

Thank you, Theresa Lech

Program Manager - Livio theresa@livio.io

[NickFromAmazon]

Could this cause issues for notifications for which order is important? OnAudioPassThru and OnKeyboardInput come to mind. If the main goal is not to block the main thread, would it make more sense to use a background serial queue?

Glancing at the code, it also appears that the current implementation of the SDLManager may be not threadsafe (I see requests are stored in a dictionary in SDLResponseDispatcher without any obvious synchronization?) I'd assume this would be a pre-req for this change, a pretty typical use case would be to make an additional SDL request in response to these notifications.

Apologies if I'm misreading the code, or this is already covered under another item that I'm missing.

[joeljfischer]

Hey @NickFromAmazon, you are correct that SDLManager is not thread safe. However, the SDLResponseDispatcher should be okay. Making an additional request in response to an SDL response would not cause any synchronization errors that I could see. The response comes in on an arbitrary thread, and the developer sends another request. I have tested this a fair bit and haven't seen any synchronization issues. If I'm missing something here, please let me know.

The only problem would be the requirement for responses that come in rapidly and need to be handled in a certain order, as the concurrent queueing, while being FIFO, will take multiple at once and place them on arbitrary threads, meaning that they could resolve out of order. OnKeyboardInput is one example, but is unlikely to cause a synchronization issue due to the length of time between keypresses – by the time a new keypress comes in, it's highly unlikely that the old OnKeyboardInput notification is waiting to be resolved. However, your OnAudioPassThru is a well taken point. I think it's unlikely again, due to the amount of time between callbacks, that the audio pass thru notifications would resolve out of order, and I have not tested this.

I think there are two possible ways forward:

1. Identify the RPC responses that require in-order notifications and send those on a serial queue, while leaving the remaining responses on a concurrent queue. This leaves some possibility of failure, but is faster.
2. As you noted, place all notifications on a background serial queue. This will reduce the performance gain, but is safer.

[NickFromAmazon]

My concern is that the data structure holding requests is not threadsafe - if it's being modified on multiple threads, there's a chance for some very hard-to-reproduce issues where different threads are getting an inconsistent view of the data.

I don't think this could be a problem only in client code, as it looks like there's also internal mutable state modified in response to RPCResponseNotifications (the SDLLifecycleManager's internal state machine seems like a particularly critical example.)

My preference would strongly be for solution 2 - I don't think it's unreasonable to expect that clients can identify when it would be advantageous to move work onto a separate thread, whereas the safer solution will likely cause far fewer issues for everyone else. My caveat here is that I'm not too familiar with the video streaming APIs, so I could be overlooking some major use cases.

My last concern with this is that it will require clients to now to provide synchronization on all SDL responses where previously none was necessary. As a part of a major version bump I don't necessarily see this being a problem as long as it's well-documented.

[kshala-ford]

Would it be a good idea to add a lifecycle configuration so the app can decide to use:

• The main queue
• A serial background queue
• A concurrent background queue
• A queue created by the app (don't know if it's needed)

The default configuration would be the main queue. Therefore it wouldn't be a breaking/risky change. For those developers who can handle code outside of the main thread can use the other options.

[NickFromAmazon]

I like this suggestion, as it would minimize impact to existing clients, and from what I've seen is a pretty familiar pattern on iOS.

Feel free to disregard my other concerns for now, as they are around implementation details.

[joeljfischer]

@kshala-ford @NickFromAmazon I'm not sure that we'd want to provide an option. That just seems to me to be burdening the developer. I don't think the main queue is the right option, it just doesn't make sense. I think it should either be a serial or concurrent queue that we control.

@NickFromAmazon I think the implementation details are at least a bit important in this case. It makes sense for very rapidly incoming RPCs to be handled serially, but everything else really can be handled concurrently. This would perhaps involve hardening a few points, as you noted. I think it's safe to move forward with using a background serial queue instead.

[NickFromAmazon]

@joeljfischer Given the existing implementation, I'd argue it would be at least as burdensome to require clients to provide synchronization where it wasn't previously necessary, especially if there is an expectation that requests back to the SDL library are made on the main queue. On the other hand, allowing clients to specify their own queue would remove the need for clients to synchronize all incoming SDL communication if they're not operating on the main queue.

But as long as requirements are well-communicated, and care is taken not to introduce any internal thread safety issues, I'd see either approach as manageable.

Unless I'm missing something, I don't imagine there would be a ton of gain to be had by using a background concurrent vs serial queue - as I mentioned above, clients should be able to identify where it's advantageous to parallelize, and as long as the SDL library isn't utilizing the same queue internally it's responsiveness shouldn't really be impacted. Further to that, I'd hypothesize that replacing any internal use ofmain for internal synchronization with a private serial queue would provide the greatest speed improvements, as it would prevent the SDL library from getting blocked by clients.

[theresalech]

The Steering Committee voted to accept this proposal with the following revision: using background serial queue instead of concurrent queue.

[theresalech]

@joeljfischer please advise when a new PR has been entered to update the proposal to reflect the agreed upon revisions. I'll then merge the PR so the proposal is up to date, and enter an issue in the iOS repository for implementation. Thanks!

[theresalech]

Proposal has been revised to include agreed upon changes and issue has been entered: [SDL 0112] SDL iOS Move RPC Notifications to a Serial Background Queue