bisakhmondal commented on a change in pull request #2054:
URL: https://github.com/apache/apisix-dashboard/pull/2054#discussion_r698493939
##########
File path: api/internal/utils/pid.go
##########
@@ -18,19 +18,62 @@
package utils
import (
+ "bytes"
"fmt"
"io/ioutil"
"os"
+ "runtime"
"strconv"
+ "syscall"
+ "time"
+
+ "github.com/pkg/errors"
)
// WritePID write pid to the given file path.
func WritePID(filepath string, forceStart bool) error {
- if pid, err := ReadPID(filepath); err == nil {
- if !forceStart {
- return fmt.Errorf("Instance of Manager API maybe
running with a pid %d. If not, please run Manager API with '-f' or '--force'
flag\n", pid)
+ pidStatAction := func() error {
+ pid, err := ReadPID(filepath)
+ if err != nil {
+ return nil
Review comment:
Hi @nic-chen , I am following a fail-fast approach. The ReadPID returns
error means it will most likely be `os.PathError` i.e. the file does not exist.
Instead of writing whole block as `if err==nil` (previous implementation), I
was trying to introduce less spaghetti code.
If you want, we definitely can improve the err to something like
```go
if err != nil {
if _, ok := err.(*os.PathError); ok {
return nil // No pid file --> no running instance
}
return err
}
```
Let me know what you think. Thanks.
##########
File path: api/internal/utils/pid.go
##########
@@ -18,19 +18,62 @@
package utils
import (
+ "bytes"
"fmt"
"io/ioutil"
"os"
+ "runtime"
"strconv"
+ "syscall"
+ "time"
+
+ "github.com/pkg/errors"
)
// WritePID write pid to the given file path.
func WritePID(filepath string, forceStart bool) error {
- if pid, err := ReadPID(filepath); err == nil {
- if !forceStart {
- return fmt.Errorf("Instance of Manager API maybe
running with a pid %d. If not, please run Manager API with '-f' or '--force'
flag\n", pid)
+ pidStatAction := func() error {
+ pid, err := ReadPID(filepath)
+ if err != nil {
+ return nil
+ }
+ // In unix system this always succeeds, doesn't ensure if the
process at all exist or not.
+ proc, err := os.FindProcess(pid)
+ if err != nil {
+ return errors.Errorf("instance of Manager API maybe
running with a pid %d. If not, please run Manager API with '-f' or '--force'
flag. Error :%v\n", pid, err)
+ }
+
+ // Traditional unix way to check the existence of the
particular PID.
+ err = proc.Signal(syscall.Signal(0))
+ // No entry of the process in process control block.
+ if err != nil {
+ return nil
Review comment:
Sorry couldn't get you. Do you mean it is okay or the same question
"why return nil here？"
If it is the latter one, the pr tries to avoid the false positive that
arises due to the stale pid file in `/tmp` (maybe that was left during shutdown
and wasn't got cleaned up)
so instead of blindly throwing an error just because of the presence of PID
file, the code block looks up if the process at all exists or not. If it
doesn't (err is not nil), that's completely fine. We will run a new instance
and overwrite the existing pid. That's it.
##########
File path: api/internal/utils/pid.go
##########
@@ -18,19 +18,62 @@
package utils
import (
+ "bytes"
"fmt"
"io/ioutil"
"os"
+ "runtime"
"strconv"
+ "syscall"
+ "time"
+
+ "github.com/pkg/errors"
)
// WritePID write pid to the given file path.
func WritePID(filepath string, forceStart bool) error {
- if pid, err := ReadPID(filepath); err == nil {
- if !forceStart {
- return fmt.Errorf("Instance of Manager API maybe
running with a pid %d. If not, please run Manager API with '-f' or '--force'
flag\n", pid)
+ pidStatAction := func() error {
+ pid, err := ReadPID(filepath)
+ if err != nil {
+ return nil
+ }
+ // In unix system this always succeeds, doesn't ensure if the
process at all exist or not.
+ proc, err := os.FindProcess(pid)
+ if err != nil {
+ return errors.Errorf("instance of Manager API maybe
running with a pid %d. If not, please run Manager API with '-f' or '--force'
flag. Error :%v\n", pid, err)
+ }
+
+ // Traditional unix way to check the existence of the
particular PID.
+ err = proc.Signal(syscall.Signal(0))
+ // No entry of the process in process control block.
+ if err != nil {
+ return nil
+ }
+
+ // Windows has a tendency to pick unused old PIDs and allocate
that to new process, so we won't kill it.
+ if runtime.GOOS != "windows" && forceStart {
+ fmt.Println("Killing existing instance of Manager API")
+ if err := proc.Signal(syscall.SIGINT); err != nil {
+ return errors.Wrapf(err, "failed to kill the
existing process of PID %d ", pid)
+ }
+ // Wait for graceful shutdown.
+ for {
+ if err := proc.Signal(syscall.Signal(0)); err
!= nil {
+ break
+ }
+ time.Sleep(time.Second)
+ }
+ fmt.Println("Existing instance of Manager API
successfully killed")
Review comment:
Cool. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
