Skip to content
This repository was archived by the owner on Feb 16, 2024. It is now read-only.

Fix for issue #190 #191

Open
shrey-gang wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Fix for issue #190 #191

Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
54ab773
#190: added feature to switch on Unix socket based on OS
shreyionos Jun 25, 2019
c74f8d0
modified code for better naming, structure and removed some duplicate…
shrey-gang Aug 25, 2019
f169105
removed TCP reference from astilectron.go, left ref in some of the co…
shrey-gang Sep 1, 2019
0224def
Merge branch 'master' into unixDomainSocket
shrey-gang Sep 8, 2019
e189c63
returning connection type(tcp/unix) and passing it to a.execute() fun…
shrey-gang Sep 11, 2019
ac66b4f
changes to send proper connection address to astilectron
shrey-gang Nov 1, 2019
0302461
chnages for connection address, address being passed without <unix/tc…
shrey-gang Nov 9, 2019
13f1a91
Merge branch 'master' into unixDomainSocket
shrey-gang Nov 9, 2019
54792fd
Merge branch 'master' into unixDomainSocket
shrey-gang Dec 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 100 additions & 40 deletions astilectron.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"os"
"os/exec"
"os/signal"
"path/filepath"
"runtime"
"sync"
"syscall"
Expand All @@ -19,9 +20,9 @@ import (

// Versions
const (
DefaultAcceptTCPTimeout = 30 * time.Second
VersionAstilectron = "0.32.0"
VersionElectron = "4.0.1"
DefaultAcceptTimeout = 30 * time.Second
VersionAstilectron = "0.32.0"
VersionElectron = "4.0.1"
)

// Misc vars
Expand All @@ -45,6 +46,11 @@ const (
EventNameAppTooManyAccept = "app.too.many.accept"
)

// // Unix socket path
Copy link
Copy Markdown
Owner

@asticode asticode Sep 18, 2019

Choose a reason for hiding this comment

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

Can you remove this?

// var (
// UNIX_SOCKET_PATH = filepath.Join(a.paths.DataDirectory(), "astilectron.sock")
// )

// Astilectron represents an object capable of interacting with Astilectron
type Astilectron struct {
canceller *asticontext.Canceller
Expand All @@ -68,16 +74,16 @@ type Astilectron struct {

// Options represents Astilectron options
type Options struct {
AcceptTCPTimeout time.Duration
AppName string
AppIconDarwinPath string // Darwin systems requires a specific .icns file
AppIconDefaultPath string
BaseDirectoryPath string
DataDirectoryPath string
ElectronSwitches []string
SingleInstance bool
SkipSetup bool // If true, the user must handle provisioning and executing astilectron.
TCPPort *int // The port to listen on.
AcceptTimeout time.Duration
AppName string
AppIconDarwinPath string // Darwin systems requires a specific .icns file
AppIconDefaultPath string
BaseDirectoryPath string
DataDirectoryPath string
ElectronSwitches []string
SingleInstance bool
SkipSetup bool // If true, the user must handle provisioning and executing astilectron.
TCPPort *int // The port to listen on.
Copy link
Copy Markdown
Owner

@asticode asticode Sep 18, 2019

Choose a reason for hiding this comment

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

Replace this attribute by Addr string so that users may provide their proper addr, not just port

}

// Supported represents Astilectron supported features
Expand Down Expand Up @@ -170,15 +176,14 @@ func (a *Astilectron) Start() (err error) {
}
}

// Unfortunately communicating with Electron through stdin/stdout doesn't work on Windows so all communications
// will be done through TCP
if err = a.listenTCP(); err != nil {
return errors.Wrap(err, "listening failed")
listenType, listenErr := a.listen()
Copy link
Copy Markdown
Owner

@asticode asticode Sep 18, 2019

Choose a reason for hiding this comment

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

Revert this part back to the original version and just rename the function from listenTCP to listen

if listenErr != nil {
return listenErr
}

// Execute
if !a.options.SkipSetup {
if err = a.execute(); err != nil {
if err = a.execute(listenType); err != nil {
Copy link
Copy Markdown
Owner

@asticode asticode Sep 18, 2019

Choose a reason for hiding this comment

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

No need for this new parameter

return errors.Wrap(err, "executing failed")
}
} else {
Expand All @@ -194,35 +199,90 @@ func (a *Astilectron) provision() error {
return a.provisioner.Provision(ctx, a.options.AppName, runtime.GOOS, runtime.GOARCH, *a.paths)
}

// listenTCP creates a TCP server for astilectron to connect to
// and listens to the first TCP connection coming its way (this should be Astilectron).
func (a *Astilectron) listenTCP() (err error) {
// function to create TCP/Unix socket connection
func (a *Astilectron) listen() (string, error) {
Copy link
Copy Markdown
Owner

@asticode asticode Sep 18, 2019

Choose a reason for hiding this comment

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

After seeing this, it seems overly complicated.

Here's what I'd rather do:

  1. Determine var addr string => check a.options.Addr first, then check os
  2. Depending on the scheme of addr, create the proper connection

/*
* Switching between Unix-Socket and TCP-Socket
* Windows will use TCP Socket
* MAC and Linux will use Unix-Socket
*/
if runtime.GOOS == "windows" {
// Unfortunately communicating with Electron through stdin/stdout doesn't
// work on Windows so all communications will be done through TCP
if err := a.listenTCP(); err != nil {
return " ", errors.Wrap(err, "TCP Socket listening failed")
}
return "tcp", nil
} else {
if err := a.listenUnix(); err != nil {
return "", errors.Wrap(err, "Unix Socket listening failed")
}
return "unix", nil
}
return "", nil
}

func (a *Astilectron) listenFunc(fn func() error) (err error) {
Copy link
Copy Markdown
Owner

@asticode asticode Sep 18, 2019

Choose a reason for hiding this comment

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

Creating this intermediary function doesn't seem really useful

// Log
astilog.Debug("Listening...")

addr := "127.0.0.1:"
if a.options.TCPPort != nil {
addr += fmt.Sprint(*a.options.TCPPort)
}
// Listen
if a.listener, err = net.Listen("tcp", addr); err != nil {
return errors.Wrap(err, "tcp net.Listen failed")
// Custom
if err = fn(); err != nil {
err = errors.Wrap(err, "main: custom listen failed")
return
}

// Check a connection has been accepted quickly enough
var chanAccepted = make(chan bool)
go a.watchNoAccept(a.options.AcceptTCPTimeout, chanAccepted)
go a.watchNoAccept(a.options.AcceptTimeout, chanAccepted)

// Accept connections
go a.acceptTCP(chanAccepted)
go a.accept(chanAccepted)
return
}

// Creates a unix socket
func (a *Astilectron) listenUnix() (err error) {

UNIX_SOCKET_PATH := filepath.Join(a.paths.DataDirectory(), "astilectron.sock")
Copy link
Copy Markdown
Owner

@asticode asticode Sep 18, 2019

Choose a reason for hiding this comment

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

Don't name the variable in upper case with _. Name it p


_ = os.Remove(UNIX_SOCKET_PATH)
Copy link
Copy Markdown
Owner

@asticode asticode Sep 18, 2019

Choose a reason for hiding this comment

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

Check the error here


if err := a.listenFunc(func() error {
Copy link
Copy Markdown
Owner

@asticode asticode Sep 18, 2019

Choose a reason for hiding this comment

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

No need for this listenFunc intermediary function

// Listen
if a.listener, err = net.Listen("unix", UNIX_SOCKET_PATH); err != nil {
return errors.Wrap(err, "Unix net.Listen failed")
}
return nil
}); err != nil {
return errors.Wrap(err, "main: listen func failed")
}
return
}

// listenTCP listens to the first TCP connection coming its way (this should be Astilectron)
func (a *Astilectron) listenTCP() (err error) {

if err = a.listenFunc(func() error {
addr := "127.0.0.1:"
if a.options.TCPPort != nil {
addr += fmt.Sprint(*a.options.TCPPort)
}
// Listen
if a.listener, err = net.Listen("tcp", addr); err != nil {
return errors.Wrap(err, "tcp net.Listen failed")
}
return nil
}); err != nil {
return errors.Wrap(err, "main: listen func failed")
}
return
}

// watchNoAccept checks whether a TCP connection is accepted quickly enough
// watchNoAccept checks whether a connection is accepted quickly enough
func (a *Astilectron) watchNoAccept(timeout time.Duration, chanAccepted chan bool) {
//check timeout
if timeout == 0 {
timeout = DefaultAcceptTCPTimeout
timeout = DefaultAcceptTimeout
}
var t = time.NewTimer(timeout)
defer t.Stop()
Expand All @@ -231,22 +291,22 @@ func (a *Astilectron) watchNoAccept(timeout time.Duration, chanAccepted chan boo
case <-chanAccepted:
return
case <-t.C:
astilog.Errorf("No TCP connection has been accepted in the past %s", timeout)
astilog.Errorf("No connection has been accepted in the past %s", timeout)
a.dispatcher.dispatch(Event{Name: EventNameAppNoAccept, TargetID: targetIDApp})
a.dispatcher.dispatch(Event{Name: EventNameAppCmdStop, TargetID: targetIDApp})
return
}
}
}

// watchAcceptTCP accepts TCP connections
func (a *Astilectron) acceptTCP(chanAccepted chan bool) {
// watchAccept accepts connections
func (a *Astilectron) accept(chanAccepted chan bool) {
for i := 0; i <= 1; i++ {
// Accept
var conn net.Conn
var err error
if conn, err = a.listener.Accept(); err != nil {
astilog.Errorf("%s while TCP accepting", err)
astilog.Errorf("%s while accepting connection", err)
a.dispatcher.dispatch(Event{Name: EventNameAppErrorAccept, TargetID: targetIDApp})
a.dispatcher.dispatch(Event{Name: EventNameAppCmdStop, TargetID: targetIDApp})
return
Expand All @@ -255,7 +315,7 @@ func (a *Astilectron) acceptTCP(chanAccepted chan bool) {
// We only accept the first connection which should be Astilectron, close the next one and stop
// the app
if i > 0 {
astilog.Errorf("Too many TCP connections")
astilog.Errorf("Too many connections")
a.dispatcher.dispatch(Event{Name: EventNameAppTooManyAccept, TargetID: targetIDApp})
a.dispatcher.dispatch(Event{Name: EventNameAppCmdStop, TargetID: targetIDApp})
conn.Close()
Expand All @@ -274,7 +334,7 @@ func (a *Astilectron) acceptTCP(chanAccepted chan bool) {
}

// execute executes Astilectron in Electron
func (a *Astilectron) execute() (err error) {
func (a *Astilectron) execute(listenType string) (err error) {
Copy link
Copy Markdown
Owner

@asticode asticode Sep 18, 2019

Choose a reason for hiding this comment

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

No need for this new parameter

// Log
astilog.Debug("Executing...")

Expand All @@ -286,7 +346,7 @@ func (a *Astilectron) execute() (err error) {
} else {
singleInstance = "false"
}
var cmd = exec.CommandContext(ctx, a.paths.AppExecutable(), append([]string{a.paths.AstilectronApplication(), a.listener.Addr().String(), singleInstance}, a.options.ElectronSwitches...)...)
var cmd = exec.CommandContext(ctx, a.paths.AppExecutable(), append([]string{a.paths.AstilectronApplication(), listenType, a.listener.Addr().String(), singleInstance}, a.options.ElectronSwitches...)...)
Copy link
Copy Markdown
Owner

@asticode asticode Sep 18, 2019

Choose a reason for hiding this comment

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

You shouldn't have to add a flag here. However make sure a.listener.Addr().String() contains the full addr (unix://path/to/socket or tcp://127.0.0.1:4003)

a.stderrWriter = astiexec.NewStdWriter(func(i []byte) { astilog.Debugf("Stderr says: %s", i) })
a.stdoutWriter = astiexec.NewStdWriter(func(i []byte) { astilog.Debugf("Stdout says: %s", i) })
cmd.Stderr = a.stderrWriter
Expand Down
⚡