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.

Sign up for GitHub

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

Jump to bottom

Move towards a more modular connection manager #385

Merged
merged 5 commits into from
Oct 15, 2018
Merged

Move towards a more modular connection manager #385

merged 5 commits into from
Oct 15, 2018

Conversation

vkuznecovas
Copy link
Contributor

@vkuznecovas vkuznecovas commented Sep 19, 2018 •
edited
Loading

Looking for feedback for this one.

updates #382

Waldz
Waldz requested changes Sep 19, 2018
View reviewed changes
core/connection/manager.go Outdated
@@ -89,14 +88,14 @@ func (manager *connectionManager) Connect(consumerID, providerID identity.Identi
}
}()

err = manager.startConnection(consumerID, providerID, options)
err = manager.start(consumerID, providerID, options)
Copy link
Member

Choose a reason for hiding this comment

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

Start what? because manager is already running.
manager.start() -> manager.startConnection() or manager.createConnection()

Copy link
Member

Choose a reason for hiding this comment

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

Maybe wo dont need 2 manager functions for start and startConnection and we can merge them?

core/connection/connect_options.go Outdated
// ConnectOptions holds connect options
type ConnectOptions struct {
// kill switch option restricting communication only through VPN
DisableKillSwitch bool
}

// Params represents the params we need to ensure a successful connection
type Params struct {
Copy link
Member

Choose a reason for hiding this comment

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

Struct looks like a duplicate of ConnectOptions.

A. Suggest to merge to ConnectOptions

B. Maybe:

  • Params -> ConnectOptions
  • ConnectOptions -> ConnectionParams -> represents plugin specific params

core/connection/connect_options.go Outdated

// Params represents the params we need to ensure a successful connection
type Params struct {
DisableKillSwitch bool
Copy link
Member

Choose a reason for hiding this comment

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

Same param

core/connection/interface.go
type VpnClientCreator func(session.SessionDto, identity.Identity, identity.Identity, state.Callback, ConnectOptions) (openvpn.Process, error)
// Connection represents a connection
type Connection interface {
Start() error
Copy link
Member

Choose a reason for hiding this comment

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

Extra line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure what you mean by that, what extra line?

core/connection/manager_test.go Outdated
}

//this is the last state - close channel (according to best practices of go - channel writer controls channel)
if openvpnState == openvpn.ProcessExited {
Copy link
Member

Choose a reason for hiding this comment

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

tests should not know about openvpn also

soffokl
soffokl reviewed Sep 20, 2018
View reviewed changes
core/connection/openvpn.go Outdated
close(channel)
}
// OpenVpnStateCallbackToConnectionState maps openvpn.State to connection.State. Returns a pointer to connection.state, or nil
func OpenVpnStateCallbackToConnectionState(input openvpn.State) *State {
Copy link
Member

Choose a reason for hiding this comment

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

Should we really return a pointer here? Returning value can make the code more readable:

        switch input {
        case openvpn.ConnectedState:
                return Connected
        case openvpn.ExitingState:
		return Disconnecting
	case openvpn.ReconnectingState:
		return Reconnecting
        default:
                return Unknown
	}

And we can introduce Unknown state to avoid nil

zolia
zolia reviewed Sep 20, 2018
View reviewed changes
core/connection/manager.go Outdated
// ErrOpenvpnProcessDied indicates that Connect method didn't reach "Connected" phase due to openvpn error
ErrOpenvpnProcessDied = errors.New("openvpn process died")
// ErrConnectionDied indicates that Connect method didn't reach "Connected" phase due to connection error
ErrConnectionDied = errors.New("connection has died")
Copy link
Contributor

Choose a reason for hiding this comment

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

Would use ConnectionFailed, since Died would imply that it was alive in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

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

ConnectionFailed

better ConnectingFailed

@vkuznecovas vkuznecovas changed the title WIP - Move towards a more modular connection manager Move towards a more modular connection manager Sep 20, 2018
Waldz
Waldz requested changes Sep 20, 2018
View reviewed changes
core/connection/interface.go Outdated

// VpnConnectionCreator creates new vpn client by given session,
// consumer identity, provider identity and uses state channel to report state changes
type VpnConnectionCreator func(ConnectOptions, StateChannel) (Connection, error)
Copy link
Member

Choose a reason for hiding this comment

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

Where should be no VPN work in core/connection package

@vkuznecovas vkuznecovas force-pushed the abstract-connection branch 3 times, most recently from 598ad67 to 69899eb Compare September 24, 2018 10:33
zolia
zolia reviewed Sep 28, 2018
View reviewed changes
services/openvpn/connection_factory.go Outdated
}

// NewOpenvpnProcessBasedConnectionFactory creates a new OpenvpnProcessBasedConnectionFactory
func NewOpenvpnProcessBasedConnectionFactory(
Copy link
Contributor

Choose a reason for hiding this comment

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

we try to get rid of New..Factory() methods, we should proceed this this at this point.

zolia
zolia reviewed Sep 28, 2018
View reviewed changes
core/connection/manager.go Outdated
manager.mutex.Lock()
defer manager.mutex.Unlock()

switch state {
case openvpn.ConnectedState:
manager.statsKeeper.MarkSessionStart()
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a bit concerned with a move of manager.statsKeeper.MarkSessionStart() to openvpn package, since it should be as a general contract to implement. Specific transport should always implement statistics collection. Most basic assumption is that each connection will have a start and a finish. Any implementation details should be hidden behind those.

zolia
zolia previously requested changes Sep 28, 2018
View reviewed changes
Copy link
Contributor

@zolia zolia left a comment

Choose a reason for hiding this comment

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

need discussion with move of stats keeper to specific transport. How those stats should relate with statistics sender? Statistics sender should always expect statistics to be available.

@vkuznecovas vkuznecovas force-pushed the abstract-connection branch 5 times, most recently from 232e8bf to 6c10259 Compare October 8, 2018 09:06
@vkuznecovas
Copy link
Contributor Author

@soffokl @Waldz any thoughts about the move of stats keeper from the manager to the connection itself?

@Waldz Waldz force-pushed the abstract-connection branch from 369aa37 to 287f010 Compare October 14, 2018 21:11
@Waldz Waldz dismissed zolia’s stale review October 15, 2018 12:44

Stale review (too long decisions)

@Waldz Waldz merged commit ce11163 into mysteriumnetwork:master Oct 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soffokl soffokl soffokl left review comments

@Waldz Waldz Waldz approved these changes

@zolia zolia zolia left review comments

@tadovas tadovas Awaiting requested review from tadovas

Assignees

@vkuznecovas vkuznecovas

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vkuznecovas @Waldz @soffokl @zolia