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-431-make-connect-method-cancelable #212

Merged
merged 17 commits into from Apr 5, 2018
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions client/connection/interface.go
Expand Up @@ -5,11 +5,12 @@ import (
"github.com/mysterium/node/identity"
"github.com/mysterium/node/openvpn"
"github.com/mysterium/node/openvpn/middlewares/state"
"github.com/mysterium/node/service_discovery/dto"
"github.com/mysterium/node/session"
)

// DialogEstablisherCreator creates new dialog establisher by given identity
type DialogEstablisherCreator func(identity.Identity) communication.DialogEstablisher
// DialogCreator creates new dialog between consumer and provider, using given contact information
type DialogCreator func(consumerID, providerID identity.Identity, contact dto.Contact) (communication.Dialog, error)

// VpnClientCreator creates new vpn client by given session,
// consumer identity, provider identity and uses state callback to report state changes
Expand Down
194 changes: 156 additions & 38 deletions client/connection/manager.go
Expand Up @@ -2,44 +2,57 @@ package connection

import (
"errors"
log "github.com/cihub/seelog"
"github.com/mysterium/node/communication"
"github.com/mysterium/node/identity"
"github.com/mysterium/node/openvpn"
"github.com/mysterium/node/openvpn/middlewares/client/bytescount"
"github.com/mysterium/node/server"
"github.com/mysterium/node/service_discovery/dto"
"github.com/mysterium/node/session"
"github.com/mysterium/node/utils"
)

const managerLogPrefix = "[connection-manager] "

const channelClosed = openvpn.State("")

var (
// ErrNoConnection error indicates that action applied to manager expects active connection (i.e. disconnect)
ErrNoConnection = errors.New("no connection exists")
// ErrAlreadyExists error indicates that aciton applieto to manager expects no active connection (i.e. connect)
ErrAlreadyExists = errors.New("connection already exists")
// ErrConnectionCancelled indicates that connection in progress was cancelled by request of api user
ErrConnectionCancelled = errors.New("connection was cancelled")
// ErrOpenvpnProcessDied indicates that Connect method didn't reach "Connected" phase due to openvpn error
ErrOpenvpnProcessDied = errors.New("openvpn process died")
)

type connectionManager struct {
//these are passed on creation
mysteriumClient server.Client
newDialogEstablisher DialogEstablisherCreator
newVpnClient VpnClientCreator
statsKeeper bytescount.SessionStatsKeeper
mysteriumClient server.Client
newDialog DialogCreator
newVpnClient VpnClientCreator
statsKeeper bytescount.SessionStatsKeeper
//these are populated by Connect at runtime
status ConnectionStatus
dialog communication.Dialog
openvpnClient openvpn.Client
sessionID session.SessionID
status ConnectionStatus
closeAction func()
}

func warnOnClose() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This private function should go after NewManager, which is public.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to the bottom

log.Warn(managerLogPrefix, "WARNING! Trying to close when there is nothing to close. Possible bug or race condition")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to start each warning with "WARNING!" - log.Warn is responsible to show that this is a warning :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

}

// NewManager creates connection manager with given dependencies
func NewManager(mysteriumClient server.Client, dialogEstablisherCreator DialogEstablisherCreator,
func NewManager(mysteriumClient server.Client, dialogCreator DialogCreator,
vpnClientCreator VpnClientCreator, statsKeeper bytescount.SessionStatsKeeper) *connectionManager {
return &connectionManager{
mysteriumClient: mysteriumClient,
newDialogEstablisher: dialogEstablisherCreator,
newVpnClient: vpnClientCreator,
statsKeeper: statsKeeper,
status: statusNotConnected(),
mysteriumClient: mysteriumClient,
newDialog: dialogCreator,
newVpnClient: vpnClientCreator,
statsKeeper: statsKeeper,
status: statusNotConnected(),
closeAction: warnOnClose,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about connectionTeardown, cleanConnection?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like cleanConnection more maybe. Renamed

}
}

Expand All @@ -55,33 +68,72 @@ func (manager *connectionManager) Connect(consumerID, providerID identity.Identi
}
}()

proposal, err := manager.findProposalByProviderID(providerID)
cancelable := utils.NewCancelable()
manager.closeAction = utils.CallOnce(func() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

manager.closeAction should be set before changing manager.status = statusConnecting() - that way we'll prevent a gap when we are in 'connecting' state but closeAction is not yet created, which will result in warnOnClose

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes there is a tiny possibility. But mutexes and locking would be better. Lets leave it to other PR

log.Info(managerLogPrefix, "Closing active connection")
manager.status = statusDisconnecting()
cancelable.Cancel()
})

val, err := cancelable.
Request(func() (interface{}, error) {
return manager.findProposalByProviderID(providerID)
}).
Call()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very unusual to keep . in the previous line - is it a common practice in Go?
I wasn't able to find these usages because I was looking for .Call() :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that go syntax style. Otherwise - syntax error :)

if err != nil {
return err
}
proposal := val.(*dto.ServiceProposal)

dialogEstablisher := manager.newDialogEstablisher(consumerID)
manager.dialog, err = dialogEstablisher.EstablishDialog(providerID, proposal.ProviderContacts[0])
val, err = cancelable.
Request(func() (interface{}, error) {
return manager.newDialog(consumerID, providerID, proposal.ProviderContacts[0])
}).
Cleanup(utils.SkipOnError(func(val interface{}) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error skipping looks complicated:

Cleanup(utils.SkipOnError(...))

cancelable.Done() - executes on success only
cancelable.Always() - executes on errors also

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it's Cleanup(utils.InvokeOnSuccess(...)) because it's not a part of cancelable request, but a function decorator which invoces callback only if error == nil

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know pattern is applied, but still complicated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

either if err == nil then do cleanup or function decorator. I would stay with decorator

val.(communication.Dialog).Close()
})).
Call()
if err != nil {
return err
}
dialog := val.(communication.Dialog)

vpnSession, err := session.RequestSessionCreate(manager.dialog, proposal.ID)
val, err = cancelable.
Request(func() (interface{}, error) {
return session.RequestSessionCreate(dialog, proposal.ID)
}).
Call()
if err != nil {
dialog.Close()
return err
}
manager.sessionID = vpnSession.ID
vpnSession := val.(*session.SessionDto)

manager.openvpnClient, err = manager.newVpnClient(*vpnSession, consumerID, providerID, manager.onVpnStatusUpdate)
stateChannel := make(chan openvpn.State, 10)
val, err = cancelable.
Request(func() (interface{}, error) {
return manager.startOpenvpnClient(*vpnSession, consumerID, providerID, stateChannel)
}).
Cleanup(utils.SkipOnError(func(val interface{}) {
val.(openvpn.Client).Stop()
})).
Call()
if err != nil {
manager.dialog.Close()
dialog.Close()
return err
}
openvpnClient := val.(openvpn.Client)

if err = manager.openvpnClient.Start(); err != nil {
manager.dialog.Close()
err = manager.waitForConnectedState(stateChannel, vpnSession.ID, cancelable.Cancelled)
if err != nil {
dialog.Close()
openvpnClient.Stop()
return err
}

go openvpnClientStopper(openvpnClient, cancelable.Cancelled)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can construct new closeAction to avoid exposing cancelable.

	manager.closeAction = utils.CallOnce(func() {
		log.Info(managerLogPrefix, "Closing active connection")
		manager.status = statusDisconnecting()
		openvpnClientStopper(openvpnClient)
	})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored. However - discussable. Do we have a better readability?

go openvpnClientWaiter(openvpnClient, dialog)
go manager.consumeOpenvpnStates(stateChannel, vpnSession.ID)
return nil
}

Expand All @@ -93,24 +145,10 @@ func (manager *connectionManager) Disconnect() error {
if manager.status.State == NotConnected {
return ErrNoConnection
}
manager.status = statusDisconnecting()
manager.openvpnClient.Stop()
manager.closeAction()
return nil
}

func (manager *connectionManager) onVpnStatusUpdate(vpnState openvpn.State) {
switch vpnState {
case openvpn.ConnectedState:
manager.statsKeeper.MarkSessionStart()
manager.status = statusConnected(manager.sessionID)
case openvpn.ExitingState:
manager.status = statusNotConnected()
manager.statsKeeper.MarkSessionEnd()
case openvpn.ReconnectingState:
manager.status = statusReconnecting()
}
}

// TODO this can be extraced as depencency later when node selection criteria will be clear
func (manager *connectionManager) findProposalByProviderID(providerID identity.Identity) (*dto.ServiceProposal, error) {
proposals, err := manager.mysteriumClient.FindProposals(providerID.Address)
Expand All @@ -123,3 +161,83 @@ func (manager *connectionManager) findProposalByProviderID(providerID identity.I
}
return &proposals[0], nil
}

func openvpnClientStopper(openvpnClient openvpn.Client, cancelRequest utils.CancelChannel) {
<-cancelRequest
log.Debug(managerLogPrefix, "Stopping openvpn client")
err := openvpnClient.Stop()
if err != nil {
log.Error(managerLogPrefix, "Failed to stop openvpn client: ", err)
}
}

func openvpnClientWaiter(openvpnClient openvpn.Client, dialog communication.Dialog) {
defer dialog.Close()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use explicit close at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

err := openvpnClient.Wait()
if err != nil {
log.Warn(managerLogPrefix, "Openvpn client exited with error: ", err)
} else {
log.Info(managerLogPrefix, "Openvpn client exited")
}
}

func (manager *connectionManager) startOpenvpnClient(vpnSession session.SessionDto, consumerID, providerID identity.Identity, stateChannel chan openvpn.State) (openvpn.Client, error) {
openvpnClient, err := manager.newVpnClient(
vpnSession,
consumerID,
providerID,
channelToStateCallbackAdapter(stateChannel),
)
if err != nil {
return nil, err
}

if err = openvpnClient.Start(); err != nil {
return nil, err
}

return openvpnClient, nil
}

func (manager *connectionManager) waitForConnectedState(stateChannel <-chan openvpn.State, sessionID session.SessionID, cancelRequest utils.CancelChannel) error {

for {
select {
case state := <-stateChannel:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very strange that state hold 2 types: State + channel close boolean

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. read state and also check if channel was closed explicitly

switch state {
case openvpn.ConnectedState:
manager.onStateChanged(state, sessionID)
return nil
case channelClosed:
return ErrOpenvpnProcessDied
default:
manager.onStateChanged(state, sessionID)
}
case <-cancelRequest:
return ErrConnectionCancelled
}
}
}

func (manager *connectionManager) consumeOpenvpnStates(stateChannel <-chan openvpn.State, sessionID session.SessionID) {
func() {
for state := range stateChannel {
manager.onStateChanged(state, sessionID)
}
manager.status = statusNotConnected()
log.Debug(managerLogPrefix, "State updater stopped")
}()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unneeded func.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed


}

func (manager *connectionManager) onStateChanged(state openvpn.State, sessionID session.SessionID) {
switch state {
case openvpn.ConnectedState:
manager.statsKeeper.MarkSessionStart()
manager.status = statusConnected(sessionID)
case openvpn.ExitingState:
manager.statsKeeper.MarkSessionEnd()
case openvpn.ReconnectingState:
manager.status = statusReconnecting()
}
}