fix: quit runwda when wda dies

[michal-przytarski]

With this changes ios runwda will exit with error when WDA app dies (e.g. killed with ios kill, killed manually with AppSwitcher, or when iPhone is turned off).
dtxConnection's BreakdownCallback is implemented with Functionals Options Pattern, so it's fully optional and shouldn't affect any other dtxConnection usages.

[dmissmann]

What we are trying to get out of this is essentially a communication between two goroutines, and that's where we should use channels in general.
I guess that could look similar to what context.Context is doing with its Done channel.

That would also allow easier error handling. Now this is done through the context, and it hides the actual problem.

[michal-przytarski]

My changes allow you to use the context.Context to manage communication between goroutines - and that’s exactly what I did.
I had a bit of trouble implementing this, because the Connection constructor itself starts a new goroutine, making it very difficult for me to put communication support there without significant changes to the public API of this module.

Can you advise me on how I should do it?

[dmissmann]

We could do this by exposing a <- chan struct{} and an error from Connection. Those can be fields on the struct, or methods (but since we're not abstracting anything from the Connection with an interface so far I guess fields on the struct are the way to start with)

type Connection struct {
    // Closed is closed when the underlying DTX connection was closed for any reason (either initiated by calling Close() or due to an error
    Closed <-chan struct{}
    // Err is non-nil when the connection was closed (when Close was called this will be io.EOF)
    Err errror

// remaining fields
}

and from the reading goroutine of a DTX connection you could then call an method like dtxConn.closedWithError(fmt.Errorf("DTX connection was closed unexpectedly: %w", err)). And in this method you have to make sure that the Closed channel gets closed exactly once (sync.Once)

If I'm not mistaken this would actually not modify anything of the existing exported APIs. Even changing the constructor newDtxConnection to a variadic one doesn't need to be done anymore as the required fields will be simply initialized in the constructor. And whoever wants to use them can do so, everyone else will also be fine with ignoring it.

My changes allow you to use the context.Context to manage communication between goroutines - and that’s exactly what I did.

It does. However, like you said the Connection constructor spins up it's own goroutine, and ideally this should receive the same context so that we can cancel it when needed. And if that would be the case this goroutine would effectively mean that this goroutine would control the context cancellation itself.
If we look at it from a hierarchical perspective, whoever creates the context is owning also saying when a context times out, or when it has to be cancelled. But with the callback you're moving this ownership to the goroutine that should be controlled by those signals

[michal-przytarski]

I've tried to implement it in the way you suggested - I've just used context instead of defining <- chan struct{} and error separately.

Does it look better now in your opinion?
22867a8

[dmissmann]

a context should never be stored in a struct (see docs)

Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it.

and you could also move the initialization dtxConnection.Ctx, dtxConnection.cancelCtx = context.WithCancelCause(context.Background()) into the constructor (just init a channel instead of the context 😉 ) to simplify it so that nobody forgets to do this step

[michal-przytarski]

a context should never be stored in a struct (see docs)

Right, but you advised me to do expose <- chan struct{} and an error from Connection which looks pretty much like storing context in a struct. I'm a little bit confused.

and you could also move the initialization dtxConnection.Ctx, dtxConnection.cancelCtx = context.WithCancelCause(context.Background()) into the constructor

So should I leave that context inside the struct? To be sure, I don't think that's a good idea.

Maybe I should just split the constructor and the call:

• remove starting reader in newDtxConnection
• add context as parameter to reader
• call reader with context form higher order function - in this case this will be runXUITestWithBundleIdsXcode15Ctx and older version runners.
• watch that context in that higher order function and check if it's Done()

This means, that the public API of package dtx will change in the breaking way. Also, this change may be difficult to detect for anyone using that package in external projects, as the connection constructor remains the same, but an additional function call will be needed to start this connection.

---

If you don't want any breaking changes, then I believe, that using the callback I've implemented in 5fc15ec to cancel higher order function context looks more "clear" (although it's not perfect)

[dmissmann]

and you could also move the initialization dtxConnection.Ctx, dtxConnection.cancelCtx = context.WithCancelCause(context.Background()) into the constructor

So should I leave that context inside the struct? To be sure, I don't think that's a good idea.

sorry, that was probably written a poorly on my end. The context should not go into the struct.
What I was trying to say was that you could move the instruction for initializing the <- chan struct{} channel into the constructor and not have it at the place where you currently have dtxConnection.Ctx, dtxConnection.cancelCtx = context.WithCancelCause(context.Background()) so that it gets always initialized and can't be forgotten

Right, but you advised me to do expose <- chan struct{} and an error from Connection which looks pretty much like storing context in a struct. I'm a little bit confused.

The outcome is similar, but context has a well stated purpose: Package context defines the Context type, which carries deadlines, cancellation signals, and other request-scoped values across API boundaries and between processes.. The reason why it looks similar is that I pretty much took how context is handling that and took only this part from it 😉

Maybe I should just split the constructor and the call:

• remove starting reader in newDtxConnection
• add context as parameter to reader
• call reader with context form higher order function - in this case this will be runXUITestWithBundleIdsXcode15Ctx and older version runners.
  watch that context in that higher order function and check if it's Done()

yes, that's also a good idea. However, it doesn't solve the problem you wanted to address in this PR, right? Because with watch that context in that higher order function and check if it's Done() you're only able to handle the cases where the caller of reader is cancelling the context, and not the reader itself.

So if you change this it would make sense to do it in a separate PR

[michal-przytarski]

I think I managed to do it the way you suggested: c1a7a70

[dmissmann]

thanks 🙂

[dmissmann]

does this have to be the last step, or why is it with defer?

[michal-przytarski]

Hmmm 🤔
I just wanted to close that channel after the internal handler is finished, but I don't think it's necessary.
I may remove defer if you wish

[michal-przytarski]

I rebased it into main to resolve merge conflicts. Now it's up to date