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

Fix async retry called from a non-main thread #1

Merged
merged 1 commit into from
Oct 10, 2016

Conversation

polac24
Copy link
Contributor

@polac24 polac24 commented Sep 7, 2016

Async retry assumed that it is always called from a main thread. While it is often a case, it could be potentially called from any thread and finalXXXX can also be called from other thread than main now. This fix synchronizes async call with a private queue that is also used to schedule retries. In addition, working on lastError inside running was dangerous.

Since now, async tests do not know when "-end" is appended to the output. I removed those expectations keeping in mind that verifying "-end" order was never crucial for async operations.

@icanzilb
Copy link
Owner

icanzilb commented Sep 7, 2016

I didn't have time to look through all changes but can you explain in more detail why did you think checking for "-end" was not crucial? This is what verifies that the execution of the code happened in the required order.

@polac24
Copy link
Contributor Author

polac24 commented Sep 7, 2016

here is scenario:

  1. We are calling tryAsync from some thread A (for instance non-main).
  2. First synchronous tryBlock throws an error so we schedule next try (for instance . immediate so in 0s) on some other thread B (we cannot assume that it should be main thread too)
  3. now we have "race": (1) threadA calls finalXXX and later appends "-end", (2) threadB schedules next try
  4. You can never say which thread wins and assert order of appending "try" (threadB was faster) or "-end" (threadA won)

@icanzilb
Copy link
Owner

I'm sorry but I can't merge the code as-is - it now explicitly changes the queue for all blocks to a custom background queue (which is as bad, if not worse as always running on the main thread) E.g. for example this PR breaks the Demo Apps and the tests ...

However, I still think the direction of the changes is the right one. I'm prepping a version of the pod for the final version of Swift 3 so I'll merge the changes and alter them to allow calling retry from the main thread to keep everything on the main thread, and calling it from another thread to use the custom operation queue you added.

I think that's a fair compromise for now, let's see if more feedback comes in from other people

@icanzilb icanzilb merged commit b4beb18 into icanzilb:master Oct 10, 2016
@icanzilb
Copy link
Owner

Thanks for checking and implementing this, maybe it'll be good to have even finer control over the threads on which each of the try, defer and catch blocks run; but I'd say for now it's enough

@polac24
Copy link
Contributor Author

polac24 commented Oct 10, 2016

I completely agree, calling on our custom background queue is dubious.
I thought about user's parameter queue:DispatchQueue (with default DispatchQueue.main) that we will use for retries.

Another question that pop up to my mind is: Don't we want to ALWAYS call "retry block" on the same thread? What bothers me that for the first time (synchronously) we call block on current (not manager by your library) thread and then next retries may arise on completely other thread. This may lead to some multithread issues and/or UI failures (calling UI related code on non UI thread).

As your suggest, let's leave it.

@icanzilb
Copy link
Owner

that's true - I think the best strategy might be to have retryAsync completely asynchronous - like put even the first try on a custom queue to reflect better the name of the function. threading is difficult 😁 I pushed today's updates to CocoaPods and I'm gonna keep an eye if there are people downloading it and develop further if there's interest

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.

None yet

2 participants