Fix for issue #190 #191

shrey-gang · 2019-06-25T17:10:15Z

#190
Partnering pull request can be found here: asticode/astilectron#20

coveralls · 2019-06-25T17:22:49Z

Coverage remained the same at ?% when pulling 54792fd on shrey-gang:unixDomainSocket into 12a267a on asticode:master.

shrey-gang · 2019-07-05T04:10:21Z

@asticode
I tried running the coverage test it doesn't fail but yes code coverage is 79% which is low.
Need some help here, can you suggest something to improve this.

asticode

Sorry for reviewing so late.

Don't bother for the tests, I'll see what I can do later.

The PR needs some changes though.

astilectron.go

…/redundant code

…mments

…ction

shrey-gang · 2019-09-11T10:30:43Z

@asticode
Sorry for taking so long in making those changes. I have made all the changes as per your suggestions.

asticode · 2019-09-18T07:27:08Z

astilectron.go

@@ -45,6 +46,11 @@ const (
 	EventNameAppTooManyAccept = "app.too.many.accept"
 )

+// // Unix socket path


Can you remove this?

asticode · 2019-09-18T07:29:25Z

astilectron.go

+	ElectronSwitches   []string
+	SingleInstance     bool
+	SkipSetup          bool // If true, the user must handle provisioning and executing astilectron.
+	TCPPort            *int // The port to listen on.


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

asticode · 2019-09-18T07:31:18Z

astilectron.go

@@ -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...)...)


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)

asticode · 2019-09-18T07:31:31Z

astilectron.go

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


No need for this new parameter

asticode · 2019-09-18T07:32:07Z

astilectron.go

 	}

 	// Execute
 	if !a.options.SkipSetup {
-		if err = a.execute(); err != nil {
+		if err = a.execute(listenType); err != nil {


No need for this new parameter

asticode · 2019-09-18T07:36:49Z

astilectron.go

+	return "", nil
+}
+
+func (a *Astilectron) listenFunc(fn func() error) (err error) {


Creating this intermediary function doesn't seem really useful

asticode · 2019-09-18T07:37:31Z

astilectron.go

+// Creates a unix socket
+func (a *Astilectron) listenUnix() (err error) {
+
+	UNIX_SOCKET_PATH := filepath.Join(a.paths.DataDirectory(), "astilectron.sock")


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

asticode · 2019-09-18T07:37:43Z

astilectron.go

+
+	UNIX_SOCKET_PATH := filepath.Join(a.paths.DataDirectory(), "astilectron.sock")
+
+	_ = os.Remove(UNIX_SOCKET_PATH)


Check the error here

asticode · 2019-09-18T07:37:59Z

astilectron.go

+
+	_ = os.Remove(UNIX_SOCKET_PATH)
+
+	if err := a.listenFunc(func() error {


No need for this listenFunc intermediary function

asticode · 2019-09-18T07:38:24Z

astilectron.go

-// 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) {


After seeing this, it seems overly complicated.

Here's what I'd rather do:

Determine var addr string => check a.options.Addr first, then check os

Depending on the scheme of addr, create the proper connection

shrey-gang · 2019-11-01T12:12:16Z

Tried testing the changes using the astilectron bundler but did not succeed in that as the bundler always downloads the astilectron from the master tested go-astilectron part the "Addr" is getting updated properly.

asticode · 2019-11-02T10:07:35Z

When using the bundler, you can use the -a flag to indicate the path to your local astilectron folder. That way, it's your astilectron version that is bundled.

…p>://<path/port>

shrey-gang · 2019-11-09T22:05:39Z

Made some changes in the client connection part, instead of checking the string for "tcp://" or "unix://" I have used net.isIP().
In this way, we don't have to make much changes in the go-astilectron part, plus the code looks cleaner.
I hope this is correct 🤞

asticode#190: added feature to switch on Unix socket based on OS

54ab773

shrey-gang mentioned this pull request Jun 25, 2019

Handling unix socket connection based on OS asticode/astilectron#21

Open

asticode suggested changes Aug 7, 2019

View reviewed changes

shrey-gang added 4 commits August 25, 2019 14:54

modified code for better naming, structure and removed some duplicate…

c74f8d0

…/redundant code

removed TCP reference from astilectron.go, left ref in some of the co…

f169105

…mments

Merge branch 'master' into unixDomainSocket

0224def

returning connection type(tcp/unix) and passing it to a.execute() fun…

e189c63

…ction

asticode suggested changes Sep 18, 2019

View reviewed changes

changes to send proper connection address to astilectron

ac66b4f

chnages for connection address, address being passed without <unix/tc…

0302461

…p>://<path/port>

shrey-gang force-pushed the unixDomainSocket branch from afe15dc to 0302461 Compare November 9, 2019 20:43

Merge branch 'master' into unixDomainSocket

13f1a91

Merge branch 'master' into unixDomainSocket

54792fd

asticode force-pushed the master branch from b331d89 to ba5c286 Compare July 27, 2023 09:39

asticode force-pushed the master branch from ba5c286 to e849f62 Compare August 8, 2023 09:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for issue #190 #191

Fix for issue #190 #191

shrey-gang commented Jun 25, 2019

coveralls commented Jun 25, 2019 •

edited

shrey-gang commented Jul 5, 2019

asticode left a comment

shrey-gang commented Sep 11, 2019

asticode Sep 18, 2019

asticode Sep 18, 2019

asticode Sep 18, 2019

asticode Sep 18, 2019

asticode Sep 18, 2019

asticode Sep 18, 2019

asticode Sep 18, 2019

asticode Sep 18, 2019

asticode Sep 18, 2019

asticode Sep 18, 2019

shrey-gang commented Nov 1, 2019

asticode commented Nov 2, 2019

shrey-gang commented Nov 9, 2019


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

		_ = os.Remove(UNIX_SOCKET_PATH)


		_ = os.Remove(UNIX_SOCKET_PATH)

		if err := a.listenFunc(func() error {

Fix for issue #190 #191

Are you sure you want to change the base?

Fix for issue #190 #191

Conversation

shrey-gang commented Jun 25, 2019

coveralls commented Jun 25, 2019 • edited

shrey-gang commented Jul 5, 2019

asticode left a comment

Choose a reason for hiding this comment

shrey-gang commented Sep 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrey-gang commented Nov 1, 2019

asticode commented Nov 2, 2019

shrey-gang commented Nov 9, 2019

coveralls commented Jun 25, 2019 •

edited