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

MYST-559 - fix forever hanging openvpn client if process exits too early #267

Merged

Conversation

Projects
None yet
3 participants
@tadovas
Copy link
Member

commented Jun 18, 2018

Key points:

  1. Move config options related stuff to separate package
  2. Merge server and client processes into single openvpn process
  3. openvpn check uses exec.Command directly instead of process wrapper
  4. Fix nasty bug when openvpn process exits too early and listener never gets incoming connection
@@ -46,7 +46,7 @@ check_uncleaned_package "github.com/mysterium/node/tequilapi" 10
check_uncleaned_package "github.com/mysterium/node/session" 6
check_uncleaned_package "github.com/mysterium/node/service_discovery/dto" 13
check_uncleaned_package "github.com/mysterium/node/server/dto" 6
check_uncleaned_package "github.com/mysterium/node/openvpn" 49
check_uncleaned_package "github.com/mysterium/node/openvpn" 35

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 21, 2018

Member

+100 karma points

//openvpn returns exit code 1 in case of --version parameter, if anything else is returned - treat as error
if exitCode != 1 {
return errors.New("unexpected openvpn code: " + strconv.Itoa(exitCode))
log.Error("[Openvpn check] ", "Check failed. Output of executed command: ", string(outputBuffer))

This comment has been minimized.

Copy link
@zolia

zolia Jun 21, 2018

Member

"Openvpn check" we could move this to separate prefix.

This comment has been minimized.

Copy link
@tadovas

tadovas Jun 21, 2018

Author Member

This is separate prefix. What do you mean?

This comment has been minimized.

Copy link
@zolia

zolia Jun 21, 2018

Member

move it to constant

This comment has been minimized.

Copy link
@tadovas

tadovas Jun 21, 2018

Author Member

Moved

if err == nil {
log.Info("[Openvpn check] ", "Custom tag: ", str)
} else if err != io.EOF {
//EOF is expected and doesn't fail check

This comment has been minimized.

Copy link
@zolia

zolia Jun 21, 2018

Member

language here does not make much sense

This comment has been minimized.

Copy link
@tadovas

tadovas Jun 21, 2018

Author Member

EOF error is expected here and EOF doesn't fail openvpn check.

This comment has been minimized.

Copy link
@tadovas

tadovas Jun 21, 2018

Author Member

Improved comment

return &CmdWrapper{
logPrefix: logPrefix,
executablePath: executablePath,
CmdExitError: make(chan error, 1), //channel should have capacity to hold single process exit error

This comment has been minimized.

Copy link
@zolia

zolia Jun 21, 2018

Member

isn't it '1' by default?

This comment has been minimized.

Copy link
@tadovas

tadovas Jun 21, 2018

Author Member

Nope. 0 by default meaning that channel is fully synchronous - write operation will not complete until someone reads from channel

type Process interface {
Start() error
Wait() error
Stop() error

This comment has been minimized.

Copy link
@zolia

zolia Jun 21, 2018

Member

error can be safely removed here, it should not be used.

@zolia

This comment has been minimized.

Copy link
Member

commented Jun 21, 2018

LGTM

tadovas added some commits Jun 7, 2018

@tadovas tadovas force-pushed the bugfix/mngmnt-listener-waits-forever-for-openvpn-process branch from 0f8cf4b to 9689195 Jun 21, 2018

@Waldz

Waldz approved these changes Jun 21, 2018

@zolia

zolia approved these changes Jun 21, 2018

@tadovas tadovas merged commit 514f337 into master Jun 21, 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 bugfix/mngmnt-listener-waits-forever-for-openvpn-process branch Jun 21, 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.