ably / ably-asset-tracking-swift

iOS client SDKs for the Ably Asset Tracking service.
Apache License 2.0
9 stars 6 forks source link

WorkerQueue core functionality #536

Closed JakubJankowski closed 1 year ago

JakubJankowski commented 1 year ago

Since the scope of the WorkerQueue refactor is really large, this PR introduces the first step in it's implementation, without actually using it anywhere in the code yet.

It introduces the Worker , WorkerFactory and WorkerQueueProperties protocols in AblyAssetTrackingInternal, and more importantly it introduces a proposal for the events queueing and handling mechanism in WorkerQueue.swift.

In general I've tried to align our approach in Swift as close to the kotlin's implementation as I could, but we obviously don't have coroutines and all that good stuff, and working with generics in swift can be a pain, so maybe we'll want to deviate from it a bit more. Any suggestions are welcome!

Relates to #523

JakubJankowski commented 1 year ago

@lawrence-forooghian One additional thing we should consider- are we okay with bumping the Xcode version requirement to 14.x? Looks like this syntax for generics in Swift that I used (which is very similar to how it looks on Android) is not available on 13.4.1 and before (which is why CI had been failing). In general we want to use recent versions of tools, but I'm not sure if forcing users to use them as well is ok. On the other hand it's been a long time since the requirement for the Xcode 12.4 was set (in Readme at least), so it might be a good opportunity to bump it.

lawrence-forooghian commented 1 year ago

@lawrence-forooghian One additional thing we should consider- are we okay with bumping the Xcode version requirement to 14.x? Looks like this syntax for generics in Swift that I used (which is very similar to how it looks on Android) is not available on 13.4.1 and before (which is why CI had been failing). In general we want to use recent versions of tools, but I'm not sure if forcing users to use them as well is ok. On the other hand it's been a long time since the requirement for the Xcode 12.4 was set (in Readme at least), so it might be a good opportunity to bump it.

@JakubJankowski please could you discuss this with @mikelee638, who is best placed to answer this from a point of view of customer impact?

JakubJankowski commented 1 year ago

Another thing I realized after talking with Kacper - the onUnexpectedError and onUnexpectedAsyncError functions on Android were introduced to capture, well, unexpected runtime exceptions. So stuff like ArrayIndexOutOfBounds, dividing by zero, and the like.

Swift doesn't allow us to catch runtime exceptions - we can only catch explicitly thrown errors. So I'm afraid those two functions can't be implemented on the iOS side. So I think we should remove them, unless there's something I'm missing.

AndyTWF commented 1 year ago

Swift doesn't allow us to catch runtime exceptions - we can only catch explicitly thrown errors. So I'm afraid those two functions can't be implemented on the iOS side. So I think we should remove them, unless there's something I'm missing.

Is it worth keeping them in place to catch exceptions that we may throw but leave otherwise un-caught? Strictly speaking onUnexpectedError and onUnexpectedAsyncError catches any exception in AAT android, including ones we might end up throwing and not handling at the time. So we could leave it in to at least gracefully handle if we're throwing somewhere and not catching in-situ?

JakubJankowski commented 1 year ago

Swift doesn't allow us to catch runtime exceptions - we can only catch explicitly thrown errors. So I'm afraid those two functions can't be implemented on the iOS side. So I think we should remove them, unless there's something I'm missing.

Is it worth keeping them in place to catch exceptions that we may throw but leave otherwise un-caught? Strictly speaking onUnexpectedError and onUnexpectedAsyncError catches any exception in AAT android, including ones we might end up throwing and not handling at the time. So we could leave it in to at least gracefully handle if we're throwing somewhere and not catching in-situ?

Hmm, I'm wondering... Maybe it's indeed worth keeping, even if for handling only "expected" errors. From the Android code documentation:

     * This function is provided in order for implementors to define what should happen when the worker
     * breaks due to an unexpected exception while the async work from [doWork] is being executed.
     * This should usually be a rollback operation and/or a call to the worker's callback function
     * with a failure with the [exception].

This should usually be a rollback operation and/or a call to the worker's callback function - this bit especially - I can imagine situations where we'll want to do a rollback after a normal error happens. So perhaps I was too hasty wanting to get rid of it altogether...

I would rename those two funcs though to onError and onAsyncError.

JakubJankowski commented 1 year ago

Swift doesn't allow us to catch runtime exceptions - we can only catch explicitly thrown errors. So I'm afraid those two functions can't be implemented on the iOS side. So I think we should remove them, unless there's something I'm missing.

Is it worth keeping them in place to catch exceptions that we may throw but leave otherwise un-caught? Strictly speaking onUnexpectedError and onUnexpectedAsyncError catches any exception in AAT android, including ones we might end up throwing and not handling at the time. So we could leave it in to at least gracefully handle if we're throwing somewhere and not catching in-situ?

Hmm, I'm wondering... Maybe it's indeed worth keeping, even if for handling only "expected" errors. From the Android code documentation:

     * This function is provided in order for implementors to define what should happen when the worker
     * breaks due to an unexpected exception while the async work from [doWork] is being executed.
     * This should usually be a rollback operation and/or a call to the worker's callback function
     * with a failure with the [exception].

This should usually be a rollback operation and/or a call to the worker's callback function - this bit especially - I can imagine situations where we'll want to do a rollback after a normal error happens. So perhaps I was too hasty wanting to get rid of it altogether...

I would rename those two funcs though to onError and onAsyncError.

Thought about it a bit more, I don't think there's much sense to use those methods after all. In Swift we have to mark methods that can throw as throws, and when we invoke them we use do-try-catch (or just try?), so, unlike in kotlin, we have to handle thrown errors where they occur. Don't think there's much point in deferring them - they should just be handled inside of an affected worker, and if some rollback needs to happen - it should happen be handled in a worker as well (or a new worker responsible for rollback could be enqueued), with a callback with an appropriate Result of course.

lawrence-forooghian commented 1 year ago

As it stands, doWork and doAsyncWork are both marked as throws, so we'd need to remove that if we wanted to remove the onUnexpectedError and onUnexpectedAsyncError methods. Perhaps we should keep them, for consistency with Android?

JakubJankowski commented 1 year ago

As it stands, doWork and doAsyncWork are both marked as throws, so we'd need to remove that if we wanted to remove the onUnexpectedError and onUnexpectedAsyncError methods. Perhaps we should keep them, for consistency with Android?

Just to not overcomplicate and move this PR forward I'll keep them for now then. If we decide they are useless we can always get rid of those funcs later on.