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

Independent main queue for Android #299

Closed
wants to merge 1 commit into from

Conversation

johnno1962
Copy link
Contributor

Hi Apple,

Please excuse the unorthodoxy of this request but this PR creates an independent main queue for Android applications. The OS's main queue is owned by the Java side of an application and work items async-queued to to current main queue are never getting executed. This change allows applications to use this pseudo main queue for synchronising work and only affects applications that are being compiled for Android where the current main queue has no meaning.

Thanks.

@MadCoder
Copy link
Member

MadCoder commented Sep 7, 2017

Then it has no meaning to do so and on Android the main queue should be made unavailable.

the main queue is a concept that allows you to have access to UI, there's no point creating one that doesn't give you that guarantee.

I would also expect that android has a way to send work to the main thread, and if it does, then the right fix is to reimplement the main queue plumbing so that it sends work on the main thread to drain the main queue, which is not unlike it is plumbed on Darwin where waking up the main queue will wakeup a port in the main CFRunloop (or on linux an eventfd) whose "handler" drains the main queue.

@MadCoder MadCoder closed this Sep 7, 2017
@parkera
Copy link
Member

parkera commented Sep 7, 2017

Our goal with swift-corelibs is to make implementation as portable as possible across platforms. A lot of code out there relies on the main queue, so simply marking it unavailable will be counterproductive.

I would argue the main queue is not entirely about UI - it is about integration with the main CFRunLoop, which exists regardless of the "UI-ness" of an application.

@johnno1962
Copy link
Contributor Author

A bit of background here... The model I’m promoting for swift in an Android app is that most of the UI work would be done in Java/Kotlin with the portable model side shared with iOS in Swift.

swiftjava

I encountered the need for a new main Queue testing the Alamofire networking framework who's delegate operations are queued to DispatchQueue.main on completion. Perhaps I also need to start up a RunLoop on a background thread to avoid other problems further down the road. For UI operations there is a runOnUiThread Java method which would be very difficult to get hold of from Swift.

@MadCoder
Copy link
Member

MadCoder commented Sep 7, 2017

@parkera I agree 100% with what you said, I said it's one option, but I would prefer making it work much better. In that regard creating a "new" queue that doesn't have the same contracts and uses an emphemeral thread is really problematic.

I think you nailed it though, we need to keep the main CFRunloop guarantee, and if the main CFRunloop works on Android, then no dispatch change is required at all.

@parkera
Copy link
Member

parkera commented Sep 7, 2017

The main CFRunLoop is part of swift-corelibs-foundation, and calls the dispatch SPI to drain the "main" queue.

I'm not sure about how we would use the main run loop on Android though. I'll defer to @johnno1962 on that.

@johnno1962
Copy link
Contributor Author

Sounds like teasing both libraries away from the “main” queue might involve a certain amount of surgery in C. Do you have any idea which areas of code might break if we don’t have a CFRunLoop running?

@parkera
Copy link
Member

parkera commented Sep 7, 2017

Foundation doesn't care about the main run loop except in one thing, which is distributed notifications. Those aren't part of swift-corelibs-foundation.

@johnno1962
Copy link
Contributor Author

On Android that wouldn’t be a problem. In-process notification centre would work though, right?

@parkera
Copy link
Member

parkera commented Sep 7, 2017

Yes, the local notification center API posts synchronously. The thing with distributed notifications is that the API promises to be delivered on the main thread (for historical reasons).

@johnno1962
Copy link
Contributor Author

OK, thanks. Is there any point in me re-opening this PR now you can see where I’m coming from? This change will have to go into the toolchain, it’s just a matter of whether it will be reflected on master.

@MadCoder
Copy link
Member

MadCoder commented Sep 7, 2017

No this pull request can't work because something has to drain the main runloop, or we have to make the main runloop unavailable and then there's little point having a main queue.

@parkera
Copy link
Member

parkera commented Sep 7, 2017

@parkera
Copy link
Member

parkera commented Sep 7, 2017

If the dispatch SPI exists (no reason not to, in my opinion), then we just use it as normal from CFRunLoop.

@parkera
Copy link
Member

parkera commented Sep 7, 2017

Then it's up to CF to decide which run loop is main. I think it'll probably be the first one that CFRunLoopGetMain is called on.

@MadCoder
Copy link
Member

MadCoder commented Sep 7, 2017

Sorry I had to leave for lunch but here are the salient points.

  • the main queue is about being drained on the main runloop.

  • the main runloop only makes sense if there's such a thing as a dedicated thread that drains it and doesn't change: people expect to see their TSDs on that thread appear reliably when on the main queue

  • the main queue has tons of weird attributes that make it unlike a regular queue (it has a prioirty, it can't be suspended and many other things) so this pull request is not the way to go anyway.

Last, if you call dispatch_main(), then the main queue becomes detached, but it was an API mistake. we should have instead booby trapped using it past dispatch_main() in the first place. Frameworks should not integract with the main queue or the main CFRunloop unless they do UI or anything that needs them. I know a lot of code does today, but it is misguided code that ought to be fixed one way or the other.

Implementing the main CFRunloop on android has to make sense because it gives you access to some feature that needs it, if not, then I don't see why making it unavailable here is a big deal, but this eventually is a decision left to porters.

The right way to do the patch you meant to write is to do what dispatch_main() does to detach the main queue and make it ask for threads, because your patch creates a new pointer which means calling into C code that would do <this queue> == dispatch_get_main_queue() would not get the expected result (on top of the wrong behavior for dispatch_set_context, the fact that you can release it, and tons of other quite important differences).

I'll let @parkera decide whether we want a main runloop on Android, but if we do, this has to be done right and given exactly the same set of guarantees you get with the one on Darwin, and there are a ton of them, way past solving the relatively easy problem of draining enqueued work.

@OlofT
Copy link

OlofT commented Sep 22, 2021

Sorry for waking up this old thread, but I am also seeking a solution to this problem, and have gone down the same line of thought as @johnno1962, and there will likely be people after me to do the same.

Have your thoughts on this developed since last?

I have tried calling CFRunLoopGetMain on a separate thread in an attempt to making that main, as suggested by @parkera. This seems to work, but I've found at least one strange behavior and that is checking for main always returns false. E.g:

DispatchQueue.main.async {
    let runLoopIsMain = CFRunLoopGetMain() === CFRunLoopGetCurrent()
    print("main? \(Thread.isMainThread) mainRunLoop: \(runLoopIsMain)")
    //becomes on Android: main? false mainRunLoop: false 
}

With this setup Android's main thread will always respond "true" when asked if it's main, even when it's not the same as DispatchMain. This makes me believe there are many other bugs/strange behaviors waiting to be discovered when using this approach. What do you think?

(And yes, I know one shouldn't build logic around checking for the current queue, but the point here is just to show irregular behavior)

For my use cases it's only necessary to have a "main" queue for coordinating UI efforts or similar (like the example above with Alamofire sending results to main). What happens afterwards is up to Android, as long we can guarantee that it will be running, and is synchronized and thread safe, we are good to go. This allows for greater code reuse between platforms.

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.

4 participants