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

More control is needed over visors started with skywiremob #472

Closed
Senyoret1 opened this issue Aug 13, 2020 · 8 comments
Closed

More control is needed over visors started with skywiremob #472

Senyoret1 opened this issue Aug 13, 2020 · 8 comments

Comments

@Senyoret1
Copy link
Contributor

Currently there are problems for controlling the visors started with skywiremob, at least to the point needed for having a good Android frontend. Here are some things that need improvements:

  • Skywiremob.stopVisor() does not completely stops the visor. After calling that function, calling Skywiremob.isVPNReady() returns true, which should not happened, as everything is supposed to be closed. Also, the console shows the [visor:shutdown]: Shutdown complete. Goodbye! message, but after some time the console periodically shows Failed to send keepalive: failed to redial underlying connection: transport is no longer being served, which also indicates that the visor is still active in some way. Trying to start the visor again causes errors like failed to add skywire networker: networker already exists to appear.

  • It is not possible to call Skywiremob.stopVisor() before the visor is totally initialized. This is problematic because the initialization process is slow and the user may want to deactivate the vpn before the whole initialization process is completed. The biggest problem is when the user does not have internet connection, because that prevents the initialization process to finish.

  • Some times more information is needed from the Visor. One example is when starting a visor without internet connection, as the initialization process may stay unresponsive for several minutes and there is no way to show progress information to the user. If it is not possible to show progress updates, being able to stop the visor at any time with skywiremob would be good, to implement a timeout in the front-end.

The problems are somewhat related and making the Skywiremob.stopVisor() function work well and making it possible to call that function at any time may be enough.

@Darkren
Copy link
Contributor

Darkren commented Aug 19, 2020

@Senyoret1 WaitForVisorToStop is this func needed at all?

@Senyoret1
Copy link
Contributor Author

It is in the skywiremob lib and the initial Android app uses it, but I don't know what it does. At least, as far as I know, the Android app does not really need it to exist, as long as skywiremob allows to stop the visor at any time. Also, something to take into account is that, as the initialization functions are synchronous and could take long to finish, the function for stopping the visor could be called from another thread

@Darkren
Copy link
Contributor

Darkren commented Aug 19, 2020

@Senyoret1 yeah, I left it there, but it really just blocks until visor exits. I'll refactor a bit, and app will be able to stop visor any time. So if you don't need to block on this, I'll just remove it

@Senyoret1
Copy link
Contributor Author

Perfect, but please make sure skywiremob has a way for the Android app to know when the visor has been stopped, as the UI needs to know that to avoid problems

@Darkren
Copy link
Contributor

Darkren commented Aug 21, 2020

@Senyoret1 hey there. Please, check out the #447 . As long as I've tested it, visor's startup may be interrupted. So, StopVisor may be called anytime. But please, try it out yourself and see if it works okay for you. Also, regarding the progress. I'm not sure how to push progress updates to the app. On visor's startup we're launching different visor modules. I guess we may build progress bar based on the amount of modules launched. But I'm not sure if that's enough. If that's enough I may try to do this, but need your opinion on this

@Senyoret1
Copy link
Contributor Author

@Darkren sorry for the late response. I tested the changes and worked well, thank you. I was able to stop the visor while in different states and start it again.

I think the only thing missing is a way to know if the visor is running. Now that waitForVisorToStop was removed, there is no way to know when the visor has been fully stopped, which is important, as the app must knows the state of all components. There are 2 alternatives:

  1. Restoring waitForVisorToStop, but this would only work if the function is callable at any point after starting the visor, not after it is fully functional.
  2. Creating a function for knowing if the visor if running.

@Darkren
Copy link
Contributor

Darkren commented Aug 31, 2020

@Senyoret1 hey there. I also forgot to mention one function. there's waitVisorReady now. which should be called after starting a visor but before preparing VPN client. it just blocks until visor gets fully initialized. you may either use this one or call one of the functions I've just added. there are isVisorStarting and isVisorRunning now. isVisorStarting returns true during visor startup, isVisorRunning starts returning true after visor startup. please tell me if anything else is needed

@Senyoret1
Copy link
Contributor Author

@Darkren thank you, the changes work well and I used them to make the most basic part of the Android app work well. The changes are in https://github.com/Darkren/skywire-mainnet/pull/1 . I have identified other changes that may be needed in skywiremob, but I still have some doubts about the details, so I will continue making changes to the Android client and file new issues if needed

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

No branches or pull requests

2 participants