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-297 service provider openvpn state reporting #157

Merged

Conversation

Projects
None yet
3 participants
@tadovas
Copy link
Member

commented Feb 13, 2018

No description provided.

statsHandler := bytescount.NewCompositeStatsHandler(statsSaver, statsSender)
asyncStatsSender := func(stats bytescount.SessionStats) error {
go statsSender(stats)
return nil

This comment has been minimized.

Copy link
@zolia

zolia Feb 14, 2018

Member

function says should return an error, but return only nil.

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 15, 2018

Author Member

Stats sender is started in separate goroutine, to avoid openvpn notifications processing blocking. There is nothing to return

@@ -71,7 +72,16 @@ func (cmd *Command) Start() (err error) {
return err
}

cmd.vpnServer = cmd.vpnServerFactory(sessionManager)
stopPinger := make(chan int)
cmd.vpnServer = cmd.vpnServerFactory(sessionManager, func(state openvpn.State) {

This comment has been minimized.

Copy link
@zolia

zolia Feb 14, 2018

Member

awkward function passing by creating inline function. Would separate anonymous function creation and pass it like a parameter, like:

callback := func(state openvpn.State) {
..
}
cmd.vpnServerFactory(sessionManager, callback)

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 15, 2018

Author Member

Fixed

@@ -35,7 +35,12 @@ func ConfigureVpnClientFactory(

statsSaver := bytescount.NewSessionStatsSaver(statsKeeper)
statsSender := bytescount.NewSessionStatsSender(mysteriumAPIClient, vpnSession.ID, signer)
statsHandler := bytescount.NewCompositeStatsHandler(statsSaver, statsSender)
asyncStatsSender := func(stats bytescount.SessionStats) error {
go statsSender(stats)

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 14, 2018

Member

In case of undelivered requests we will form a queue of hanging routines.
Can You create a ticket for smarted async middlewares.

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 15, 2018

Author Member

Created MYST-338

time.Sleep(1 * time.Minute)
cmd.mysteriumClient.PingProposal(proposal, signer)
select {
case <-time.After(1 * time.Minute):

This comment has been minimized.

Copy link
@zolia

zolia Feb 14, 2018

Member

I would externalise various timeouts as config parameters, like:

const commandServerPingTimeout := 1 * time.Minute

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 15, 2018

Author Member

Yes, but this is not timeout. It's ping period. And it's used in one place only

@@ -83,8 +93,18 @@ func (cmd *Command) Start() (err error) {
}
go func() {
for {
time.Sleep(1 * time.Minute)
cmd.mysteriumClient.PingProposal(proposal, signer)
select {

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 14, 2018

Member

Is there a possibility to make controller thiner?

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 15, 2018

Author Member

I am not sure, I understood the question.

@tadovas tadovas force-pushed the feature/MYST-297-service-provider-openvpn-state-reporting branch from 9039693 to 0afee65 Feb 14, 2018

connection := &fakeConnection{}
middleware.Start(connection)
mockWritter := &management.MockConnection{}
middleware.Start(mockWritter)

This comment has been minimized.

Copy link
@zolia

zolia Feb 14, 2018

Member

should be mockWriter

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 15, 2018

Author Member

Actually it should be mockConnection. Fixed, thanks for pointing out


middleware := &middleware{}
stateTracker := &stateTracker{}
middleware.Subscribe(stateTracker.recordState)

This comment has been minimized.

Copy link
@zolia

zolia Feb 14, 2018

Member

nice one! 👍

@zolia

zolia approved these changes Feb 15, 2018

@tadovas tadovas merged commit ecbb774 into master Feb 15, 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/MYST-297-service-provider-openvpn-state-reporting branch Feb 15, 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.