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

Change Postal completion handlers queue #34

Open
klefevre opened this issue Sep 29, 2016 · 6 comments
Open

Change Postal completion handlers queue #34

klefevre opened this issue Sep 29, 2016 · 6 comments
Assignees
Labels
Milestone

Comments

@klefevre
Copy link
Contributor

Today we dispatch results on the main thread because we assume that Postal is mostly used for UI purpose which is not necessarily true.

It's just an idea but I think we should remove this behaviour because if we use Postal for processing purpose we may not want Postal to enqueue on the main queue (even if here we are talking about very lightweight messages). Users might want to save some computations from the main-thread before actually displaying something to the UI.

So I would propose to drop this behaviour. As an example NSURLSession doesn't dispatch on the main thread even if the most common usage of it is to display something fetched from internet on the UI. And if we think about it. Postal and NSURLSession do, in a sense, the same kind of work...

As an alternative, we could add an option to the Postal instance or adding a parameter to methods that have a completion handler to let the choice to dispatch results on the main thread or not.

@jeremiegirault
Copy link
Contributor

Like NSURLSession, I think we should create a "Configuration".
The default configuration would setup the dispatch on main queue.
A custom configuration could allow the client to define its own dispatch queue.
What do you think ?

Should we consider using an output OperationQueue like NSURLSession does or stay on DispatchQueue ? We will have to keep an internal serial DispatchQueue because libetpan is not thread safe at all, but we can propose the user to define an output OperationQueue.

@klefevre klefevre added this to the 0.4 milestone Sep 30, 2016
@klefevre
Copy link
Contributor Author

klefevre commented Sep 30, 2016

I like your idea of a "Configuration" struct but we already have a "Configuration" parameter in the initializer related to the IMAP session. I don't like the idea of putting things like these into it.

I would suggest instead adding a simple parameter in the initializer. It would look like this:

open class Postal {
     public init(configuration: Configuration, completionQueue: OperationQueue = .main)

@jeremiegirault
Copy link
Contributor

Disagree, in order to future proof the API

We should indeed have a ProviderConfiguration.
We should have another struct for PostalConnection Options (separation of concerns).
We are in breaking mode so we can allow ourselves to change this, But let's assume that other options will emerge.

@klefevre
Copy link
Contributor Author

klefevre commented Oct 3, 2016

Ok fair enough. Have you an idea of what should be added in this new structure aside the completion queue ?

@jeremiegirault
Copy link
Contributor

At the moment the timeout parameter is provided "ad-hoc" in the connect func :
https://github.com/snipsco/Postal/blob/master/Postal/Postal.swift#L65
All of these tweaks Which are note provider specific could go there.

@klefevre
Copy link
Contributor Author

klefevre commented Oct 7, 2016

To answer your question. OperationQueue didn't seems to have change a lot for a while unlike DispatchQueue so in my opinion DispatchQueue would be better. Moreover I think that most of developers use it in a first place and switch to OperationQueue for a very specific case.

However we have to remember that we used an OperationQueue for libetpan because of a bug of dispatch_async that was sending some messages to the main thread..

EDIT: Personally I would like to switch to a full DispatchQueue implementation. WDYT?

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

2 participants