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

Prevent calling API on Bluejay while a background task is running #84

Merged
merged 2 commits into from
Jul 14, 2017

Conversation

sakuraehikaru
Copy link

It is best to only work with the synchronized peripheral while inside a background task. It is usually a programmer error to try to access something from the Bluejay interface while inside a background task, or an attempt to try to use the background task for something it is not designed for.

This PR adds some protection against doing things like calling bluejay.write/read/listen, or even bluejay.scan and bluejay.disconnect inside a background task. Doing so will either result in the incorrect calls failing immediately with the appropriate error, or not doing anything at all. In the latter case, a warning is printed to the console.

Additionally, I've added protection against calling run:backgroundTask:... simultaneously, or in close succession before the first one finishes. Only one execution of a background task is allowed at a time for simplicity.

@sakuraehikaru sakuraehikaru self-assigned this Jul 11, 2017
@nbrooke
Copy link
Member

nbrooke commented Jul 11, 2017

It seems like this is doing something slightly different than what your description says it is, not sure if that is intentional or not, or if I am misunderstanding.

It seems like there are two situations this prevents stuff in:

  • Trying to call regular functions while INSIDE a background task. This is what it seems like it is mainly trying to prevent.
  • Calling regular functions from the main thread while there is also a background task running. I'm not sure if preventing this is intentional (other than the specifically mentioned case of disallowing starting a second background task)

The former, is definitely 100% wrong and won't ever work. That a serious enough programmer error, I think having that be a fatal error might even be appropriate.

The latter I'm less confident is wrong. There might be technical reasons that it doesn't work properly now, but it seems like that could in theory be supported correctly, and I can definitely imagine use cases for. However, it sort of reminds me of our discussion of multiple listens on the same characteristic, in that even though "good" uses are possible, any given use of is more likely to be unintentional / a mistake, so if we were going to support it, it seems like it should probably be an error by default and have an opt in way to turn the error off.

That would require a totally separate implementation for doing the errors in the two cases though.

@sakuraehikaru
Copy link
Author

sakuraehikaru commented Jul 11, 2017

@nbrooke regular and synchronized functions both use the same queue. If we don't block calling regular functions on the main thread, and especially regular functions that will enqueue an operation while a background task is running, Bluejay could potentially execute commands that don't follow the order specified in the background task – for example, if a background task is supposed to run A, B, C, and a main thread call enqueues D while the background task is running, then depending on the timing of things, D could be inserted anywhere in between A, B, and C, and that might not be ideal.

@nbrooke
Copy link
Member

nbrooke commented Jul 11, 2017

Yeah, this is potentially dangerous, and should be an error by default. But you can have use case that are reasonable when you know the order of the task doesn't matter for a specific set of operations.

As a concrete example, say you have a device that has a bunch of data collected on the device than can be synced in one of the standard "big sync" operations we'd use background task for and also has a battery level that is displayed on one screen in the app. You kick off a sync, start navigating around while the sync is running and get to the screen that has the battery indicator on it. In that case reading the battery level with a normal read from that view controller while the sync was going on in the background would be totally reasonable, because you know in that case there is no dependancies.

Question is, is there a good way to allow that, without making it less likely to be safe in the default case? Best I can come up with is to have a initialization option to allow this. So if you set "allowBackgroundOverlap" or something at create time, it disables these errors in the cases that might be safe (i.e. it would still always be an error to call them from inside a background task). I'm just not sure how important it is to bother with that. Maybe the right thing to do is just go with this for now and look into allowing that if we ever need it or if other people complain enough.

@sakuraehikaru
Copy link
Author

I've filed an issue to track building support for calling regular Bluejay API on the main thread while a background task is running.

Copy link
Member

@nbrooke nbrooke left a comment

Choose a reason for hiding this comment

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

That sounds reasonable to me.

@sakuraehikaru
Copy link
Author

@nbrooke I've expanded the check, so that now it will crash if regular Bluejay operations are called from the same thread as the running background task. If called from any other threads, they won't crash, but will continue to either fail immediately or do nothing but log a warning to console.

if isRunningBackgroundTask {
    // Terminate the app if this is called from the same thread as the running background task.
    Dispatch.dispatchPrecondition(condition: .notOnQueue(DispatchQueue.global()))
    completion(.failure(Error.backgroundTaskRunning()))
    return
}

@sakuraehikaru sakuraehikaru merged commit a3fa74b into master Jul 14, 2017
@sakuraehikaru sakuraehikaru deleted the 60-prevent-not-using-synchronized-peripheral branch July 14, 2017 20:08
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.

2 participants