Calling start() moves state to CONNECTING

The expected behavior is to go from STARTING_STOPPING to DISABLED it seems, which does seem a bit weird from an external library user perspective to be honest.

assigned to @akwizgran

Looks like this was carried across from Briar's TorPlugin. I seem to have added the networkInitialised flag in commit briar@b2a1ea84 in order to create this special case of returning ENABLING (which corresponds to onionwrapper's CONNECTING) if enableNetwork() has never been called. But why?

Looking also at briar@242d6f8a, I wonder if the goal was to enter the ENABLING state as quickly as possible, so that in the common case where Tor was enabled, the Tor icon would quickly switch to amber and then eventually to green. Without the networkInitialised flag, the icon would remain grey while the Tor process was starting and Briar was connecting to the control port, and would only become amber when enableNetwork() was called.

If I'm right then this workaround doesn't belong in a library, but if we remove it then we'll need to find another way of achieving the same behaviour in Briar.

In this case, wouldn't renaming CONNECTING to STARTING be a reasonable solution?

If I'm right then this workaround doesn't belong in a library

I am not sure about this. There's this weird part of the start method where we could already have bootstrapped or built circuits. So if there's issue that can come up during starting, then maybe having a way to know Tor is in the starting state is a good thing, to also have in the library?

Onionshare currently uses that starting state to know when it can enable the network. Or could this also be done right after calling start() instead?

The CONNECTING state is also used when the Tor process has finished starting and is connecting to the network, and we can revert to that state later if we lose our bridge/guard connections. So I don't think renaming CONNECTING to STARTING would be right.

I do agree though that it would be useful for the library to distinguish between these states. Perhaps we should add a new STARTING state while the Tor process is starting? The wrapper would then transition to DISABLED until enableNetwork() was called, at which point it would transition to CONNECTING.

From Briar's point of view there might be a flicker in the UI, as we went from STARTING (amber) to DISABLED (grey) and then CONNECTING (amber). Perhaps this is Briar's problem and not onionwrapper's, though - or could it also cause issues for Mailbox or OnionShare?

If it did cause issues then we could keep the current workaround, where the state is considered to be CONNECTING until enableNetwork() has been called for the first time. So the state would transition from STARTING (amber) to CONNECTING (amber) with no flicker. Strictly speaking this seems incorrect, as the network is disabled by default, and it's misleading to report a CONNECTING state if the app hasn't enabled the network - but if it avoids problems for our existing apps I guess it might be the pragmatic solution.

Onionshare currently uses that starting state to know when it can enable the network. Or could this also be done right after calling start() instead?

Right after start() would work.

From Briar's point of view there might be a flicker in the UI, as we went from STARTING (amber) to DISABLED (grey) and then CONNECTING (amber).

That is because we call enableNetwork only with some (visible) delay?

Right after start() would work.

In that case, it would never even go to DISABLED, right? In mailbox we also call enableNetwork() right after calling start() (if we are online). So I guess there wouldn't be an issue then.

Btw. I find it clearer to rename this to NETWORK_DISABLED, if something like this is still possible.

That is because we call enableNetwork only with some (visible) delay?

We call it asynchronously when start() returns, so we'll broadcast an event for the DISABLED state followed by an event for the CONNECTING state and the delay may or may not be noticeable.

In that case, it would never even go to DISABLED, right?

It would, because we're not calling these methods on the UI thread.

So what does that mean? You'd prefer to leave the current workaround in place and stay with connecting even though nothing is connecting when enableNetwork hasn't been called?

Personally, if the UI flicker is the only issue then I'd be in favour of removing the workaround, but if removing it would also cause issues for the other apps then maybe we should keep it. So I'd suggest checking whether OnionShare and Mailbox react to DISABLED differently from CONNECTING, and if not then we know the flicker would be the only issue.

But it's a judgement call. What do you think?

Since both apps are calling enableNetwork immediately after starting and have a different UI, I think it wouldn't be an issue there. The mailbox currently has an internal inactive state when we treat as ErrorNoNetwork, but we could simply change this, maybe even get rid of that state.

Come to think of it, if we're adding a new STARTING state then could we keep the workaround, but just return STARTING rather than CONNECTING if enableNetwork() hasn't been called yet? This would avoid the flicker without misleadingly broadcasting CONNECTING when the network was not yet enabled.

That would require OnionShare to call enableNetwork() when start() returns, but apart from that it seems like it would work for all the current apps?

I think this would work. We should just add docs to this new starting state about this behavior, because it is still slightly confusing. But then, who would start Tor without (soonish) also enable the network?

To make this less confusing I added STARTING and STARTED states (as well as docs).

mentioned in merge request !9 (merged)

mentioned in commit 2ee68f44

closed with merge request !9 (merged)