Multi-instance Process Compose #873
Conversation
internal/services/manager.go
Outdated
|
|
||
| lockFile(config.File.Fd()) | ||
| defer config.File.Close() | ||
| defer unlockFile(config.File.Fd()) |
There was a problem hiding this comment.
this does first unlock, and then file-close. 🤔 I think this is problematic because:
- process#1: unlock happens
- process#2: starts writing to the same file
- process#1: closes file, which flushes any system buffers to disk. This clobbers writes of process#2.
There was a problem hiding this comment.
-u, --unlock
Drop a lock. This is usually not required, since a lock is
automatically dropped when the file is closed.
source: https://man7.org/linux/man-pages/man1/flock.1.html
So, this makes me think that you just want defer config.File.Close().
There was a problem hiding this comment.
I'd also suggest checking the error return values of config.File.Close() and doing:
defer func() {
err := config.File.Close()
if err != nil {
// prefer writer since we manage the --quiet flag using that, but if its hard to get a writer here
// for now, you could do io.Stderr
fmt.Fprintf(writer /* or io.Stderr */, "failed to close file (%s). Got error %v\n", someFilePath, err)
}
}
This way we may get alerted to any errors from conflicts, etc. with our file-locking-and-writing code.
There was a problem hiding this comment.
also, maybe move the defer file.config.Close() to be after the getGlobalConfig's error check?
There was a problem hiding this comment.
Ah that makes sense, if I can unlock the file by closing it, then I can just defer that and only call unlock specifically when I need it
| } | ||
|
|
||
| defer os.Remove(processComposePidfile) | ||
| return cmd.Wait() | ||
| err = cmd.Wait() |
There was a problem hiding this comment.
Do we want to get the output of the command, (i.e. out, err := cmd.Output()) which we could then print in the error message below?
There was a problem hiding this comment.
I don't think the command really outputs anything, since this starts the TUI.
|
|
||
| config := readGlobalProcessComposeJSON(configFile) |
There was a problem hiding this comment.
comment on line 294 can be moved up or removed
| } | ||
| return nil | ||
|
|
||
| case <-time.After(fileLockTimeout): |
There was a problem hiding this comment.
ah, this is a nice way to do a timeout
internal/services/manager.go
Outdated
| if err != nil { | ||
| return err | ||
| return globalConfigFile, fmt.Errorf("failed to open config file: %w", err) |
There was a problem hiding this comment.
nit: I'd return nil in these error cases.
| } | ||
|
|
||
| cmd := exec.Command(processComposeBinPath, flags...) | ||
| return runProcessManagerInForeground(cmd, port, projectDir, w) | ||
| return runProcessManagerInForeground(cmd, config, port, projectDir, w) |
There was a problem hiding this comment.
this feels a bit problematic:
- above we are doing
defer configFile.Close() - Inside
runProcessManagerInForeground, we are closing the file, waiting for the command, and re-opening a file.
I worry that since the step (1) defer runs after step (2),then it (step 1) will error because it is closing a file that's already closed.
Oh, but we are implicitly discarding the error from file.Close() in the defer block, so all good I think!
There was a problem hiding this comment.
I guess another option is to defer the entire cleanup block after cmd.Wait call + error check in runProcessManagerInForeground to ensure that it runs -- but I agree that discarding the error from defer configFile.Close() avoids the issue as well.
Summary
This PR allows users to run process-compose for multiple devbox projects at the same time, without any collisions or undesirable behavior. This does not provide any isolation between services in different projects, but it does let a user effectively manage them on a per project basis
How it works
When starting process-compose in a devbox project, we create a UUID for the project, and then store the port + PID of the process-compose instance in a global
process-compose.jsonfile, with the UUID as the key.When making client requests to process-compose, a devbox project looks up the UUID for the project in the .devbox directory, gets the right port, and then makes the request. This ensures each project is communicating with the correct process-compose instance to start and stop services.
Devbox also uses the same lookup to get the PID of a project's process-compose instance, so it can stop the right instance.
Questions:
Future features:
devbox services upAdddevbox services stop --all, which stops all running process-compose files and resets the global config. This could help with situations where a developer accidentally leaves their projects running.How was it tested?
Ran Example Tests to check backward compatibility, tested by running postgres + jekyll simultaneously, starting and stopping them