Milestone 1 - Core Functionality #1

jmbarzee · 2021-03-19T08:27:44Z

Add bitbox.Core
Offers process manipulation and output based on uuids.

Add proc.Proc
Wrapper around os/exec.Cmd which offers

process control
Routes process output to temp files.
Offers streams of output by file polling until the process has closed.

…ulated after the command exits.

…re" normal ProcStatus.

proc/proc.go

core.go

NajiObeid · 2021-03-19T14:45:19Z

proc/proc.go

+			case <-ticker.C:
+				// ReadLoop
+				for {
+					b, err := ioutil.ReadAll(bufFile)


it might not be a good idea to put the whole file in memory

Hahaha, yes. I think I rushed this implementation over from some quick testing I did. I'll clean that up. Does reading till new lines make sense to you or would you rather see a fixed number of bytes? (I lean towards #2)

yes fixed number bytes is the way to go 👍

proc/proc.go

NajiObeid · 2021-03-19T15:13:25Z

proc/proc.go

+// Proc is a BitBox process.
+// Stderr and Stdout are dumped to temp files on disk.
+type Proc struct {
+	// TODO: there is 99% chance we need a synchronization primitive at this level


can you explain why we might/not need the sync primitive?

Late nights make concurrent code extra hard. Let me walk through my thoughts and see what it leads to. I'm not certain about either direction yet.

My thoughts were that __FileNames are only ever written during NewProc where concurrent access isn't yet possible. After that cmd could have:

Concurrent calls to Wait()

Reads of cmd.ProcessState

Reads of cmd.process for os.Process.Kill()

Concurrent calls to Wait()

I am certain we need a synchronization primitive because of lines 502-505 in the definition of cmd.Wait() in os/exec/exec.go

If a routine is preempted after if c.finished { but before c.finished = true two routines would attempt to visit the resource clean up section of the function.

Reads of cmd.ProcessState

I think we should be good here. ProcessState is a pointer that should be set with a single instruction during Wait() so it should be safe for concurrent access.

Reads of cmd.process for os.Process.Kill()

I think a mutex is definitely needed to protect cmd.Wait(), but if cmd.process.Kill() is protected by the same mutex the process will never be able to be killed before it has exited. With that said, I think cmd.process.Kill() is safe for concurrent calls because the method makes several naked calls until it reaches Process.signal() in os/exec_unix.go where its protected by a lock on line 69. So I think we are good there.

Okay. I've protected cmd.Wait with a mutex. Anything else you feel is pending here @NajiObeid?

proc/proc.go

NajiObeid · 2021-03-19T17:30:39Z

proc/proc.go

+	if err := pollRead(ctx, p.stdoutFileName, wg, stream, newProcOutput_Stdout); err != nil {
+		cancel()
+		return nil, fmt.Errorf("failed to setup poll read on stdout: %w", err)
+	}
+	if err := pollRead(ctx, p.stderrFileName, wg, stream, newProcOutput_Stderr); err != nil {
+		cancel()
+		return nil, fmt.Errorf("failed to setup poll read on stderr: %w", err)
+	}


this comes hand in hand with my previous comment about joining both outputs

Yes, I think you've made an excellent point! I responded to you there.

proc/proc.go

wadells

This may have been covered by Naji's remarks: In the Query logic I'm concerned about termination/leaks in the case of a failure to read everything from the returned <-chan ProcOutput. This is the the "client reads from second 2-5 of 10s of output" case discussed earlier. It looks like the code will load data into memory, and then block forever on the send, if nothing nothing receives on the far end. This seems like a risk of both a goroutine & memory leak -- if I understand the logic correctly.

proc/proc.go

wadells · 2021-03-19T23:27:55Z

proc/proc.go

+	if p.cmd.ProcessState.ExitCode() != 0 {
+		return Stopped
+	}
+	return Exited


Based on your comments below, this logic doesn't look correct to me. The docs say:

ExitCode returns the exit code of the exited process, or -1 if the process hasn't exited or was terminated by a signal.

https://golang.org/pkg/os/#ProcessState.ExitCode

Whereas you say:

// Stopped indicates that the process returned no exit code.

Great catch. It took me a minute to realize where my brain was hung up here. I changed the definitions of Stopped and Exited and the check to be in line with ExitCode() because that made the most sense to me. How does it look to you now?

core.go

proc/proc.go

core.go

jmbarzee · 2021-03-19T19:34:08Z

proc/proc.go

+// Proc is a BitBox process.
+// Stderr and Stdout are dumped to temp files on disk.
+type Proc struct {
+	// TODO: there is 99% chance we need a synchronization primitive at this level


Late nights make concurrent code extra hard. Let me walk through my thoughts and see what it leads to. I'm not certain about either direction yet.

My thoughts were that __FileNames are only ever written during NewProc where concurrent access isn't yet possible. After that cmd could have:

Concurrent calls to Wait()

Reads of cmd.ProcessState

Reads of cmd.process for os.Process.Kill()

proc/proc.go

jmbarzee · 2021-03-19T19:46:14Z

proc/proc.go

+// Proc is a BitBox process.
+// Stderr and Stdout are dumped to temp files on disk.
+type Proc struct {
+	// TODO: there is 99% chance we need a synchronization primitive at this level


Concurrent calls to Wait()

I am certain we need a synchronization primitive because of lines 502-505 in the definition of cmd.Wait() in os/exec/exec.go

If a routine is preempted after if c.finished { but before c.finished = true two routines would attempt to visit the resource clean up section of the function.

jmbarzee · 2021-03-19T22:02:57Z

proc/proc.go

+	if err := pollRead(ctx, p.stdoutFileName, wg, stream, newProcOutput_Stdout); err != nil {
+		cancel()
+		return nil, fmt.Errorf("failed to setup poll read on stdout: %w", err)
+	}
+	if err := pollRead(ctx, p.stderrFileName, wg, stream, newProcOutput_Stderr); err != nil {
+		cancel()
+		return nil, fmt.Errorf("failed to setup poll read on stderr: %w", err)
+	}


Yes, I think you've made an excellent point! I responded to you there.

proc/proc.go

jmbarzee · 2021-03-19T22:08:18Z

proc/proc.go

+			case <-ticker.C:
+				// ReadLoop
+				for {
+					b, err := ioutil.ReadAll(bufFile)


Hahaha, yes. I think I rushed this implementation over from some quick testing I did. I'll clean that up. Does reading till new lines make sense to you or would you rather see a fixed number of bytes? (I lean towards #2)

core.go

wadells

I think there are still some areas for improvement (notably concurrency handling in query), but I think integrating this logic with a grpc server and client will help bring them to light. I'd like to move forward to that point, so we have a bit more holistic view of the system.

Jacob Barzee added 16 commits March 17, 2021 18:37

add line to gitignore

8ec989f

Add unimplemented core

ec3b391

add proc package for handling processes and their output.

04e3134

make Core start, stop, and store processes

bc082b5

move bitbox.ProcStatus to proc.ProcStatus

bd5bcd8

implement proc.Status(),

792fc71

implement core.Status(id)

9a2465c

call exec.Cmd.Wait to ensure that cmd.ProcessState is going to be pop…

3cc6a18

…ulated after the command exits.

Change order of ProcStatus enum to communicate that Exited is the "mo…

4eebc6b

…re" normal ProcStatus.

Fix ridiculous typo. Lock() should be Unlock()

a8522b7

move ProcOutput to proc package

11d7c4c

Fix proc.ProcStatus.String()

dd689d7

Add ProcOutput constructors

5bbc5bd

Implement proc.Proc.Query() and BitBox.Query()

4f6c0e7

Add missing returns for errors case.

b4e5d12

Change proc to hold file names for output instead of file handles

7aeaa6d

jmbarzee force-pushed the milestone/1-core-functionality branch from 0ecb392 to 7aeaa6d Compare March 19, 2021 08:43

dboslee reviewed Mar 19, 2021

View reviewed changes

proc/proc.go Outdated Show resolved Hide resolved

dboslee reviewed Mar 19, 2021

View reviewed changes

core.go Outdated Show resolved Hide resolved