Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added a graceful shutdown method, added -spawnChild mode #10

Merged
merged 12 commits into from
Sep 23, 2019
5 changes: 2 additions & 3 deletions .travis.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
language: go

go:
- 1.8.x
- 1.9.x
- tip
- 1.12.x
- 1.13.x
77 changes: 76 additions & 1 deletion tika/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"os"
"os/exec"
"time"
"strconv"

"golang.org/x/net/context/ctxhttp"
)
Expand All @@ -39,6 +40,41 @@ type Server struct {
url string // url is derived from port.
port string
cmd *exec.Cmd
child *ChildOptions
}

// ChildOptions represent command line parameters that can be used when Tika is run with the -spawnChild option.
// If a field is less than or equal to 0, the associated flag is not included.
type ChildOptions struct {
MaxFiles int
TaskPulseMillis int
TaskTimeoutMillis int
PingPulseMillis int
PingTimeoutMillis int
}

func (co *ChildOptions) args() []string {
if co == nil {
return nil
}
args := []string{}
args = append(args, "-spawnChild")
if co.MaxFiles == -1 || co.MaxFiles > 0 {
args = append(args, "-maxFiles", strconv.Itoa(co.MaxFiles))
}
if co.TaskPulseMillis > 0 {
args = append(args, "-taskPulseMillis", strconv.Itoa(co.TaskPulseMillis))
}
if co.TaskTimeoutMillis > 0 {
args = append(args, "-taskTimeoutMillis", strconv.Itoa(co.TaskTimeoutMillis))
}
if co.PingPulseMillis > 0 {
args = append(args, "-pingPulseMillis", strconv.Itoa(co.PingPulseMillis))
}
if co.PingTimeoutMillis > 0 {
args = append(args, "-pingTimeoutMillis", strconv.Itoa(co.PingTimeoutMillis))
}
return args
}

// URL returns the URL of this Server.
Expand Down Expand Up @@ -67,14 +103,25 @@ func NewServer(jar, port string) (*Server, error) {
return s, nil
}

// ChildMode sets up the server to use the -spawnChild option.
// If used, ChildMode must be called before starting the server.
// If you want to turn off the -spawnChild option, call Server.ChildMode(nil).
func (s *Server) ChildMode(ops *ChildOptions) error {
if s.cmd != nil {
return fmt.Errorf("server process already started, cannot switch to spawn child mode")
}
s.child = ops
return nil
}

var command = exec.Command

// Start starts the given server. Start will start a new Java process. The
// caller must call Stop() to shut down the process when finished with the
// Server. Start will wait for the server to be available or until ctx is
// cancelled.
func (s *Server) Start(ctx context.Context) error {
cmd := command("java", "-jar", s.jar, "-p", s.port)
cmd := command("java", append([]string{"-jar", s.jar, "-p", s.port}, s.child.args()...)...)

if err := cmd.Start(); err != nil {
return err
Expand Down Expand Up @@ -113,6 +160,8 @@ func (s Server) waitForStart(ctx context.Context) error {
// Stop shuts the server down, killing the underlying Java process. Stop
// must be called when finished with the server to avoid leaking the
// Java process. If s has not been started, Stop will panic.
// If not running in a Windows environment, it is recommended to use Shutdown
// for a more graceful shutdown of the Java process.
func (s *Server) Stop() error {
if err := s.cmd.Process.Kill(); err != nil {
return fmt.Errorf("could not kill server: %v", err)
Expand All @@ -123,6 +172,32 @@ func (s *Server) Stop() error {
return nil
}

// Shutdown attempts to close the server gracefully before using SIGKILL,
Copy link
Member

Choose a reason for hiding this comment

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

Should we change the behavior of Stop and add a context.Context argument? If someone wants to kill immediately, they can pass a context.Context that is Done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can but that would make this update not backwards compatible. I added a Shutdown Method in order to not conflict with Stop. Also the golang documentation, on sending signals in the "os" package, warns that Sending Interrupt is not implemented in Windows. (https://golang.org/pkg/os/#Process.Signal). So currently Shutdown would not be advised in a Windows environment.

Yes, passing a context.Context that is Done would currently send a SIGINT signal followed immediately by a SIGKILL. That was the intended behavior of Shutdown.

// Stop() uses SIGKILL right away, which causes the kernal to stop the java process instantly.
func (s *Server) Shutdown(ctx context.Context) error {
if err := s.cmd.Process.Signal(os.Interrupt); err != nil {
return fmt.Errorf("could not interrupt server: %v", err)
}
errChannel := make(chan error)
go func() {
select {
case errChannel <- s.cmd.Wait():
case <-ctx.Done():
}
}()
select {
Copy link
Member

Choose a reason for hiding this comment

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

Could this all be a single select statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your right, I'll change this, Was originally thinking that I was going to need two threads to handle the Signalling and error handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually no, went to implement this change and realized that s.cmd.Wait() runs synchronously. So s.cmd.Wait() can't be a case in a select statement, I need to wrap it inside it's own go routine to make it asynchronous with the context.Context.

Copy link
Member

Choose a reason for hiding this comment

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

Ah. That definitely makes sense. Mind adding a comment to Stop referencing Shutdown?

case err := <- errChannel:
if err != nil {
return fmt.Errorf("could not wait for server to finish: %v", err)
}
case <-ctx.Done():
if err := s.cmd.Process.Kill(); err != nil {
return fmt.Errorf("could not kill server: %v", err)
}
}
return nil
}

func sha512Hash(path string) (string, error) {
f, err := os.Open(path)
if err != nil {
Expand Down