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

Feature/myst-227 openvpn client async state changes #147

Merged
merged 25 commits into from Feb 8, 2018

Conversation

Projects
None yet
4 participants
@tadovas
Copy link
Member

commented Feb 6, 2018

No description provided.

@tadovas tadovas force-pushed the feature/MYST-227-openvpn-client-async-state-changes branch from ee6efa6 to 5781624 Feb 6, 2018

if err := manager.vpnClient.Start(); err != nil {
manager.status = statusError(err)
if err := manager.openvpnClient.Start(); err != nil {
manager.dialog.Close()

This comment has been minimized.

Copy link
@zolia

zolia Feb 7, 2018

Member

shouldn't we close session too?

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 7, 2018

Author Member

What session?

This comment has been minimized.

Copy link
@zolia

zolia Feb 7, 2018

Member

ok, its just session DTO


assert.False(tc.T(), tc.fakeStatsKeeper.SessionStartMarked)
assert.False(tc.T(), tc.fakeStatsKeeper.sessionStartMarked)
}

func (tc *testContext) TestOnConnectErrorStatusIsNotConnectedAndLastErrorIsSetAndSessionStartIsNotMarked() {

This comment has been minimized.

Copy link
@zolia

zolia Feb 7, 2018

Member

I don't see where LastErrorIsSet is checked.

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 7, 2018

Author Member

Good point. Since there is no "last error" anymore, test will be renamed.

if err := cmd.Process.Kill(); err != nil {
//First - shutdown gracefully
//TOOD - add timer and send SIGKILL after timeout?
if err := cmd.Process.Signal(syscall.SIGTERM); err != nil {
return

This comment has been minimized.

Copy link
@zolia

zolia Feb 7, 2018

Member

investigate if we can print / pass error somehow.

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 7, 2018

Author Member

Maybe at least print, because we are in shutdown phase already.

status: statusNotConnected(),
mysteriumClient: mysteriumClient,
newDialogCreator: dialogEstablisherFactory,
newVpnClient: vpnClientFactory,

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 7, 2018

Member

Several different names looks weird: ..Factory, ..Creator

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 7, 2018

Author Member

It should be Creator. Renamed


proposals, err := manager.mysteriumClient.FindProposals(providerID.Address)
proposal, err := manager.findProposalByNode(providerID)

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 7, 2018

Member

What is that node? :)

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 7, 2018

Author Member

What node? :) renamed to findProposalByProviderId

auth.NewMiddleware(credentialsProvider),
), nil
// TODO this can be extraced as depencency later when node selection criteria will be clear
func (manager *connectionManager) findProposalByNode(providerID identity.Identity) (*dto.ServiceProposal, error) {

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 7, 2018

Member

I always saw discovery interface like that:

mysteriumClient.FindProposals(filters) []Proposal
mysteriumClient.GetProposal(providerID) Proposal

What do you thing about moving this function to mysteriumClient.GetProposal()?

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 7, 2018

Author Member

Possible. More refactoring to do. Not sure if scope of this PR

@@ -100,7 +100,7 @@ func (cmd *Command) Wait() error {
func (cmd *Command) Kill() error {
err := cmd.connectionManager.Disconnect()
if err != nil {
return err
fmt.Printf("Disconnect error: %s\n", err)

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 7, 2018

Member

Should error be eaten? If so, not clear why

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 7, 2018

Author Member

In case of disconnect when no connection exists, we return NoConnection error, we cannot make a return here, because it will not stop application itself. Just print message and continue

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 7, 2018

Author Member

According to our discussion. Special case with NoConnection is needed to handle here and otherwise pass error upstairs.

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 7, 2018

Author Member

Fixed.

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 7, 2018

Member

I agree stopp yelling in validation errors.
But still where are serious errors and they'are ignored

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 7, 2018

Author Member

Please check updated version

@@ -102,6 +101,12 @@ func (management *Management) serveNewConnection(connection net.Conn) {
middleware.Start(connection)
}

defer func() {

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 7, 2018

Member

Should not we start all waitForShutdown at defer?

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 7, 2018

Author Member

connection cleanup moved to separate fuction for better readability

if err := cmd.Process.Kill(); err != nil {
return
//First - shutdown gracefully
//TOOD - add timer and send SIGKILL after timeout?

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 7, 2018

Member

Mistype

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 7, 2018

Author Member

Fixed

"github.com/mysterium/node/session"
)

type State string
type DialogEstablisherCreator func(identity identity.Identity) communication.DialogEstablisher

This comment has been minimized.

Copy link
@donce

donce Feb 7, 2018

Contributor

Docs missing.

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 7, 2018

Author Member

Done

SessionID session.SessionID
LastError error
}
type VpnClientCreator func(session.SessionDto, identity.Identity, state.ClientStateCallback) (openvpn.Client, error)

This comment has been minimized.

Copy link
@donce

donce Feb 7, 2018

Contributor

Docs missing.

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 7, 2018

Author Member

Done

status ConnectionStatus
dialog communication.Dialog
openvpnClient openvpn.Client
sessionId session.SessionID

This comment has been minimized.

Copy link
@donce

donce Feb 7, 2018

Contributor

sessionID

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 7, 2018

Author Member

Done

Reconnecting = State("Reconnecting")
)

type ConnectionStatus struct {

This comment has been minimized.

Copy link
@donce

donce Feb 7, 2018

Contributor

Doc missing.

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 7, 2018

Author Member

Done

Connected = State("Connected")
Disconnecting = State("Disconnecting")
Reconnecting = State("Reconnecting")
)

This comment has been minimized.

Copy link
@donce

donce Feb 7, 2018

Contributor

All public const should have docs.

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 7, 2018

Author Member

Done

@tadovas

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2018

@Waldz @donce comments reviewed, fixed

@@ -20,6 +20,7 @@ type Management struct {

listenerShutdownStarted chan bool
listenerShutdownWaiter sync.WaitGroup
once sync.Once

This comment has been minimized.

Copy link
@donce

donce Feb 7, 2018

Contributor

once is very generic and doesn't specify it's purpose - closeOnce?

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 7, 2018

Author Member

as per our discusion - closesOnce

func ConfigureVpnClientFactory(
mysteriumAPIClient server.Client,
configDirectory string,
runtimeDirectory string,

This comment has been minimized.

Copy link
@donce

donce Feb 7, 2018

Contributor

These could be shortened: configDirectory, runtimeDirectory string :)

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 7, 2018

Author Member

Shortened.

@@ -38,6 +39,7 @@ func NewManagement(socketAddress, logPrefix string, middlewares ...ManagementMid

listenerShutdownStarted: make(chan bool),
listenerShutdownWaiter: sync.WaitGroup{},
once: sync.Once{},

This comment has been minimized.

Copy link
@donce

donce Feb 7, 2018

Contributor

I believe this line can be ommited - it initiates it by default.

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 7, 2018

Author Member

True. Removed

if err := cmd.Process.Kill(); err != nil {
return
//First - shutdown gracefully
//TODO - add timer and send SIGKILL after timeout?

This comment has been minimized.

Copy link
@donce

donce Feb 7, 2018

Contributor

cmd.Process.Kill() (which was removed) can be used if gracefull shutdown fails - no need for SIGKILL. From docs:

Kill causes the Process to exit immediately.

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 7, 2018

Author Member

This TODO is more about killing process immediately after a delay (in case graceful shutdown fails), and yes send(SIGKILL) and Kill() are equivalent. I think it could be added with separate PR

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 7, 2018

Member

Is it hard to implement delayed force kill?
At least, lets create ticket, or we will forget it soon

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 8, 2018

Author Member

Ticket was created.

@@ -100,7 +100,7 @@ func (cmd *Command) Wait() error {
func (cmd *Command) Kill() error {
err := cmd.connectionManager.Disconnect()
if err != nil {
return err
fmt.Printf("Disconnect error: %s\n", err)

This comment has been minimized.

Copy link
@donce

donce Feb 7, 2018

Contributor

Why not using logger?

@Waldz

Waldz approved these changes Feb 7, 2018

@donce

donce approved these changes Feb 8, 2018

@tadovas tadovas merged commit ac3cf6b into master Feb 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tadovas tadovas deleted the feature/MYST-227-openvpn-client-async-state-changes branch Feb 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.