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

Conversation

Projects
None yet
3 participants
@tadovas
Copy link
Member

commented Mar 29, 2018

No description provided.

closeAction func()
}

func warnOnClose() {

This comment has been minimized.

Copy link
@donce

donce Apr 3, 2018

Contributor

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

This comment has been minimized.

Copy link
@tadovas

tadovas Apr 3, 2018

Author Member

Moved to the bottom


// SkipOnError decorator takes callback function with single val as parameter and returns CleanupCallback function, which calls
// given callback only if no error was returned by blocking operation (i.e. good when cleanup is need only when blocking operation succeded but was Cancelled before)
func SkipOnError(callback func(interface{})) CleanupCallback {

This comment has been minimized.

Copy link
@donce

donce Apr 3, 2018

Contributor

First though when reading hame is that this is empty cleanup callback - it simple does nothing (skips callback).
Maybe InvokeOnSuccess?

This comment has been minimized.

Copy link
@tadovas

tadovas Apr 3, 2018

Author Member

Nice naming. Fixed

}

// Request method takes long running operation function as parameter and returns copy of CancelableAction as a result
func (c CancelableAction) Request(method BlockingOperation) CancelableAction {

This comment has been minimized.

Copy link
@donce

donce Apr 3, 2018

Contributor

Request and Cleanup names seem like they perform request and cleanup - when in fact they don't, they just set the values. What about SetRequestActionor OnRequest? Or maybe builder pattern is better here, extracting these methods to a separate class which is responsible for building an object will Request and Cleanup already set?

This comment has been minimized.

Copy link
@tadovas

tadovas Apr 3, 2018

Author Member

As per our discussion. Cancelable is just cancel channel wrapper. It has method NewRequest which constructs actual cancelable request. Refactored


type actionResultChannel chan actionResult

func callBlockingAction(blockingOp BlockingOperation, cleanup CleanupCallback, cancel CancelChannel) (interface{}, error) {

This comment has been minimized.

Copy link
@donce

donce Apr 3, 2018

Contributor

Maybe cancel should be named cancelled, since channel on this side is used for checking state, not as an action?

This comment has been minimized.

Copy link
@tadovas

tadovas Apr 3, 2018

Author Member

Good point. Renamed

Request(func() (interface{}, error) {
return manager.findProposalByProviderID(providerID)
}).
Call()

This comment has been minimized.

Copy link
@donce

donce Apr 3, 2018

Contributor

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

This comment has been minimized.

Copy link
@tadovas

tadovas Apr 3, 2018

Author Member

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

@@ -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() {

This comment has been minimized.

Copy link
@donce

donce Apr 3, 2018

Contributor

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

This comment has been minimized.

Copy link
@tadovas

tadovas Apr 3, 2018

Author Member

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

}

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

This comment has been minimized.

Copy link
@donce

donce Apr 3, 2018

Contributor

Use explicit close at the end.

This comment has been minimized.

Copy link
@tadovas

tadovas Apr 3, 2018

Author Member

Fixed

}
manager.status = statusNotConnected()
log.Debug(managerLogPrefix, "State updater stopped")
}()

This comment has been minimized.

Copy link
@donce

donce Apr 3, 2018

Contributor

Unneeded func.

This comment has been minimized.

Copy link
@tadovas

tadovas Apr 3, 2018

Author Member

Removed

return err
}

go openvpnClientStopper(openvpnClient, cancelable.Cancelled)

This comment has been minimized.

Copy link
@donce

donce Apr 3, 2018

Contributor

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)
	})

This comment has been minimized.

Copy link
@tadovas

tadovas Apr 3, 2018

Author Member

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

}

func warnOnClose() {
log.Warn(managerLogPrefix, "WARNING! Trying to close when there is nothing to close. Possible bug or race condition")

This comment has been minimized.

Copy link
@donce

donce Apr 3, 2018

Contributor

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

This comment has been minimized.

Copy link
@tadovas

tadovas Apr 3, 2018

Author Member

Removed

assert.Fail(tc.T(), "Connect error not expected: ", err)
default:
assert.Equal(tc.T(), statusConnecting(), tc.connManager.Status())
}

This comment has been minimized.

Copy link
@donce

donce Apr 3, 2018

Contributor

We can use boolean for that - getting boolean value from variable and setting it are atomic operations AFAIK.

func (tc *testContext) TestStatusReportsConnectingWhenConnectionIsInProgress() {
	tc.fakeOpenVpn.onStartReportStates = []openvpn.State{}

	connected := false
	go func() {
		tc.connManager.Connect(myID, activeProviderID)
		connected = true
	}()

	waitABit()
	assert.Equal(tc.T(), statusConnecting(), tc.connManager.Status())
	assert.False(tc.T(), connected)
}

This comment has been minimized.

Copy link
@donce

donce Apr 3, 2018

Contributor
func (tc *testContext) TestStatusReportsConnectingWhenConnectionIsInProgress() {
	tc.fakeOpenVpn.onStartReportStates = []openvpn.State{}

	go func() {
		tc.connManager.Connect(myID, activeProviderID)
		assert.Fail(tc.T(), "Connect finished unexpectedly")
	}()

	waitABit()
	assert.Equal(tc.T(), statusConnecting(), tc.connManager.Status())
}
waitABit()
select {
case err := <-errorChannel:
assert.Fail(tc.T(), "Connect error not expected: ", err)

This comment has been minimized.

Copy link
@donce

donce Apr 3, 2018

Contributor

Invalid params - Fail does not accept error params.

This comment has been minimized.

Copy link
@tadovas

tadovas Apr 3, 2018

Author Member

Removed

@@ -42,8 +43,9 @@ func NewCommandWith(

identityManager := identity.NewIdentityManager(keystoreInstance)

dialogEstablisherFactory := func(myID identity.Identity) communication.DialogEstablisher {
return nats_dialog.NewDialogEstablisher(myID, identity.NewSigner(keystoreInstance, myID))
dialogFactory := func(consumerID, providerID identity.Identity, contact dto.Contact) (communication.Dialog, error) {

This comment has been minimized.

Copy link
@Waldz

Waldz Apr 3, 2018

Member
  • It's dialogCreator
  • When establishing new dialog to some peer, You should know only providerID & providerContact of that peer

This comment has been minimized.

Copy link
@tadovas

tadovas Apr 3, 2018

Author Member

This dialogCreator is called from client connection manager, where consumer and provider is always know. Simply do one function call to create dialog instead of two inside connection manager

dialogEstablisherFactory := func(myID identity.Identity) communication.DialogEstablisher {
return nats_dialog.NewDialogEstablisher(myID, identity.NewSigner(keystoreInstance, myID))
dialogFactory := func(consumerID, providerID identity.Identity, contact dto.Contact) (communication.Dialog, error) {
dialogEstablisher := nats_dialog.NewDialogEstablisher(consumerID, identity.NewSigner(keystoreInstance, consumerID))

This comment has been minimized.

Copy link
@Waldz

Waldz Apr 3, 2018

Member

Why new dialogEstablisher struct is created for each dialog?

This comment has been minimized.

Copy link
@tadovas

tadovas Apr 3, 2018

Author Member

That's because when connecting we can provide different identities

newVpnClient: vpnClientCreator,
statsKeeper: statsKeeper,
status: statusNotConnected(),
closeAction: warnOnClose,

This comment has been minimized.

Copy link
@Waldz

Waldz Apr 3, 2018

Member

What about connectionTeardown, cleanConnection?

This comment has been minimized.

Copy link
@tadovas

tadovas Apr 3, 2018

Author Member

I like cleanConnection more maybe. Renamed

Request(func() (interface{}, error) {
return manager.newDialog(consumerID, providerID, proposal.ProviderContacts[0])
}).
Cleanup(utils.SkipOnError(func(val interface{}) {

This comment has been minimized.

Copy link
@Waldz

Waldz Apr 3, 2018

Member

This error skipping looks complicated:

Cleanup(utils.SkipOnError(...))

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

This comment has been minimized.

Copy link
@tadovas

tadovas Apr 3, 2018

Author Member

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

This comment has been minimized.

Copy link
@Waldz

Waldz Apr 5, 2018

Member

I know pattern is applied, but still complicated

This comment has been minimized.

Copy link
@tadovas

tadovas Apr 5, 2018

Author Member

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

#!/usr/bin/env bash

echo "Starting"
sleep 0.2

This comment has been minimized.

Copy link
@donce

donce Apr 4, 2018

Contributor

why is 100-milisec-process doing 200ms sleep? :D

This comment has been minimized.

Copy link
@tadovas

tadovas Apr 4, 2018

Author Member

Nice catch :) fixed

cmdShutdownStarted: make(chan bool),
cmdShutdownWaiter: sync.WaitGroup{},
}
}

// Process struct define process wrapper which handles clean shutdown, tracks executable exit errors, etc.

This comment has been minimized.

Copy link
@donce

donce Apr 4, 2018

Contributor

defines*

This comment has been minimized.

Copy link
@tadovas

tadovas Apr 4, 2018

Author Member

Fixed

@@ -10,7 +10,7 @@ import (
// CheckOpenvpnBinary function checks that openvpn is available, given path to openvpn binary
func CheckOpenvpnBinary(openvpnBinary string) error {

process := NewProcess(openvpnBinary, "[openvpn binary check]")
process := NewProcess(openvpnBinary, "[openvpn binary check] ")

This comment has been minimized.

Copy link
@donce

donce Apr 4, 2018

Contributor

This is out of scope of this PR, but I hate that we're doing this prefix formatting of "[%s] " in so many places - it should be done in one place :/

This comment has been minimized.

Copy link
@tadovas

tadovas Apr 4, 2018

Author Member

Well [] it's just nice surround. I don't see any problem not to use [ ] at all.

This comment has been minimized.

Copy link
@donce

donce Apr 5, 2018

Contributor

Using it is good, it increases readability - but I think this formatting should be done in one place so that we could easily change that. Not this formatting is duplicated across whole system.

return nil
}

func (foc *fakeOpenvpnClient) reportState(state openvpn.State) {
foc.stateCallback(state)
time.Sleep(time.Millisecond)

This comment has been minimized.

Copy link
@Waldz

Waldz Apr 5, 2018

Member

wut?

This comment has been minimized.

Copy link
@tadovas

tadovas Apr 5, 2018

Author Member

Removed - not needed anyway. Every test itself now does waiting for state changes as long as its needed.


for {
select {
case state := <-stateChannel:

This comment has been minimized.

Copy link
@Waldz

Waldz Apr 5, 2018

Member

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

This comment has been minimized.

Copy link
@tadovas

tadovas Apr 5, 2018

Author Member

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

if err := openvpnClient.Stop(); err != nil {
log.Warn(managerLogPrefix, "Openvpn process stopped with error: ", err)
} else {
log.Info(managerLogPrefix, "Openvpn client stopped")

This comment has been minimized.

Copy link
@Waldz

Waldz Apr 5, 2018

Member

Unify Openvpn process .. with Openvpn client ..

This comment has been minimized.

Copy link
@tadovas

tadovas Apr 5, 2018

Author Member

Fixed

@Waldz

Waldz approved these changes Apr 5, 2018

@tadovas tadovas merged commit e30e31f into master Apr 5, 2018

2 checks passed

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

@tadovas tadovas deleted the feature/cancelable-connections branch Apr 5, 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.