Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 18 additions & 10 deletions signals.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,22 +18,30 @@ const signalBufferSize = 2048
// while still forwarding all other signals to the process.
// If notifySocket is present, use it to read systemd notifications from the container and
// forward them to notifySocketHost.
func newSignalHandler(enableSubreaper bool, notifySocket *notifySocket) *signalHandler {
func newSignalHandler(enableSubreaper bool, notifySocket *notifySocket) chan *signalHandler {
if enableSubreaper {
// set us as the subreaper before registering the signal handler for the container
if err := system.SetSubreaper(1); err != nil {
logrus.Warn(err)
}
}
// ensure that we have a large buffer size so that we do not miss any signals
// in case we are not processing them fast enough.
s := make(chan os.Signal, signalBufferSize)
// handle all signals for the process.
signal.Notify(s)
return &signalHandler{
signals: s,
notifySocket: notifySocket,
}
handler := make(chan *signalHandler)
// signal.Notify is actually quite expensive, as it has to configure the
// signal mask and add signal handlers for all signals (all ~65 of them).
// So, defer this to a background thread while doing the rest of the io/tty
// setup.
go func() {
Comment thread
cyphar marked this conversation as resolved.
// ensure that we have a large buffer size so that we do not miss any
// signals in case we are not processing them fast enough.
s := make(chan os.Signal, signalBufferSize)
// handle all signals for the process.
signal.Notify(s)
handler <- &signalHandler{
signals: s,
notifySocket: notifySocket,
}
}()
return handler
}

// exit models a process exit status with the pid and
Expand Down
3 changes: 2 additions & 1 deletion utils_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ func (r *runner) run(config *specs.Process) (int, error) {
// Setting up IO is a two stage process. We need to modify process to deal
// with detaching containers, and then we get a tty after the container has
// started.
handler := newSignalHandler(r.enableSubreaper, r.notifySocket)
handlerCh := newSignalHandler(r.enableSubreaper, r.notifySocket)
tty, err := setupIO(process, r.container, config.Terminal, detach, r.consoleSocket)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Did you see if doing setupIO or setupPidfdSocket in a goroutine also provides a speed-up? I guess they're both not very complicated...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I test these two paths, both of them spend only several milliseconds on doing the setup tasks.
I checked the code, they only connect to a socket or run a small loop for stdio, so I think it's not worth to run them in a new go routine.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fair enough. Thanks!

if err != nil {
return -1, err
Expand Down Expand Up @@ -285,6 +285,7 @@ func (r *runner) run(config *specs.Process) (int, error) {
return -1, err
}
}
handler := <-handlerCh
status, err := handler.forward(process, tty, detach)
if err != nil {
r.terminate(process)
Expand Down