Skip to content

Commit

Permalink
Use ulimit to check the open files soft limit (#66)
Browse files Browse the repository at this point in the history 
**What is changing**: Use the value returned from `ulimit -n` as the
open files soft limit instead of the value returned from the syscall
`getrlimit`.

**Why this change is being made**: Since Go 1.19, during initialization,
Go programs unconditionally set a high open files soft limit for
themselves without modifying the system-wide defaults. This soft limit
also does not apply to subprocesses run via the `exec` package. Because
`wkhtmltopdf` is run as a subprocess, the soft limit set by the Go
program during initialization does not give us any information about the
limits that will be applied to `wkhtmltopdf`. Two approaches would be to
1. always call `setrlimit` on every run so that the soft limit will
apply to subprocesses, or
2. use `ulimit` to check the soft limit that will be applied to
`wkhtmltopdf`.

I chose to implement approach 2. to reduce the number of syscalls.

More details:
- https://go.dev/src/syscall/rlimit.go
- golang/go#46279
- https://stackoverflow.com/q/73640931/5403337
-
https://www.perplexity.ai/search/explain-why-golang-returns-an-tho.xk6ARhOs6ZSPr4kx8A

**Related issue(s)**: Fixes #63

**Follow-up changes needed**: None

**Is the change completely covered by unit tests? If not, why not?**:
Yes
  • Loading branch information
@tagatac
tagatac authored Sep 12, 2024
1 parent 8d1118a commit b3b93a0
Show file tree
Hide file tree
Showing 8 changed files with 208 additions and 104 deletions.
3 changes: 1 addition & 2 deletions cmd/bagoup/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import (
"github.com/tagatac/bagoup/v2/chatdb"
"github.com/tagatac/bagoup/v2/internal/bagoup"
"github.com/tagatac/bagoup/v2/opsys"
"github.com/tagatac/bagoup/v2/opsys/scall"
"github.com/tagatac/bagoup/v2/pathtools"
)

Expand All @@ -57,7 +56,7 @@ func main() {
logFatalOnErr(errors.Wrap(err, "create pathtools"))
opts.DBPath = ptools.ReplaceTilde(opts.DBPath)

s, err := opsys.NewOS(afero.NewOsFs(), os.Stat, scall.NewSyscall())
s, err := opsys.NewOS(afero.NewOsFs(), os.Stat)
logFatalOnErr(errors.Wrap(err, "instantiate OS"))
db, err := sql.Open("sqlite3", opts.DBPath)
logFatalOnErr(errors.Wrapf(err, "open DB file %q", opts.DBPath))
Expand Down
3 changes: 1 addition & 2 deletions example-exports/examplegen.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (
"github.com/spf13/afero"
"github.com/tagatac/bagoup/v2/opsys"
"github.com/tagatac/bagoup/v2/opsys/pdfgen"
"github.com/tagatac/bagoup/v2/opsys/scall"
)

type parameters struct {
Expand All @@ -39,7 +38,7 @@ func main() {
{isPDF: false, exportPath: "messages-export"},
{isPDF: true, exportPath: "messages-export-pdf"},
}
s, err := opsys.NewOS(afero.NewOsFs(), os.Stat, scall.NewSyscall())
s, err := opsys.NewOS(afero.NewOsFs(), os.Stat)
if err != nil {
log.Panic(errors.Wrap(err, "instantiate OS"))
}
Expand Down
20 changes: 10 additions & 10 deletions internal/bagoup/export_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,14 +66,14 @@ func TestExportChats(t *testing.T) {
osMock.EXPECT().Create("messages-export/testdisplayname/testguid;;;testguid2.txt").Return(chatFile, nil),
osMock.EXPECT().NewTxtOutFile(chatFile).Return(ofMocks[0]),
ofMocks[0].EXPECT().Stage(),
osMock.EXPECT().GetOpenFilesLimit().Return(256),
osMock.EXPECT().GetOpenFilesLimit().Return(256, nil),
ofMocks[0].EXPECT().Flush(),
dbMock.EXPECT().GetMessageIDs(3),
osMock.EXPECT().MkdirAll("messages-export/testdisplayname2", os.ModePerm),
osMock.EXPECT().Create("messages-export/testdisplayname2/testguid3.txt").Return(chatFile, nil),
osMock.EXPECT().NewTxtOutFile(chatFile).Return(ofMocks[1]),
ofMocks[1].EXPECT().Stage(),
osMock.EXPECT().GetOpenFilesLimit().Return(256),
osMock.EXPECT().GetOpenFilesLimit().Return(256, nil),
ofMocks[1].EXPECT().Flush(),
)
},
Expand Down Expand Up @@ -116,7 +116,7 @@ func TestExportChats(t *testing.T) {
osMock.EXPECT().Create("messages-export/testdisplayname/testguid;;;testguid2.txt").Return(chatFile, nil),
osMock.EXPECT().NewTxtOutFile(chatFile).Return(ofMocks[0]),
ofMocks[0].EXPECT().Stage(),
osMock.EXPECT().GetOpenFilesLimit().Return(256),
osMock.EXPECT().GetOpenFilesLimit().Return(256, nil),
ofMocks[0].EXPECT().Flush(),
)
},
Expand Down Expand Up @@ -159,14 +159,14 @@ func TestExportChats(t *testing.T) {
osMock.EXPECT().Create("messages-export/testdisplayname/testguid;;;testguid2.txt").Return(chatFile, nil),
osMock.EXPECT().NewTxtOutFile(chatFile).Return(ofMocks[0]),
ofMocks[0].EXPECT().Stage(),
osMock.EXPECT().GetOpenFilesLimit().Return(256),
osMock.EXPECT().GetOpenFilesLimit().Return(256, nil),
ofMocks[0].EXPECT().Flush(),
dbMock.EXPECT().GetMessageIDs(3),
osMock.EXPECT().MkdirAll("messages-export/testdisplayname2", os.ModePerm),
osMock.EXPECT().Create("messages-export/testdisplayname2/testguid3.txt").Return(chatFile, nil),
osMock.EXPECT().NewTxtOutFile(chatFile).Return(ofMocks[1]),
ofMocks[1].EXPECT().Stage(),
osMock.EXPECT().GetOpenFilesLimit().Return(256),
osMock.EXPECT().GetOpenFilesLimit().Return(256, nil),
ofMocks[1].EXPECT().Flush(),
)
},
Expand Down Expand Up @@ -199,14 +199,14 @@ func TestExportChats(t *testing.T) {
osMock.EXPECT().Create("messages-export/testdisplayname/testguid.txt").Return(chatFile, nil),
osMock.EXPECT().NewTxtOutFile(chatFile).Return(ofMocks[0]),
ofMocks[0].EXPECT().Stage(),
osMock.EXPECT().GetOpenFilesLimit().Return(256),
osMock.EXPECT().GetOpenFilesLimit().Return(256, nil),
ofMocks[0].EXPECT().Flush(),
dbMock.EXPECT().GetMessageIDs(2),
osMock.EXPECT().MkdirAll("messages-export/testdisplayname", os.ModePerm),
osMock.EXPECT().Create("messages-export/testdisplayname/testguid2.txt").Return(chatFile, nil),
osMock.EXPECT().NewTxtOutFile(chatFile).Return(ofMocks[1]),
ofMocks[1].EXPECT().Stage(),
osMock.EXPECT().GetOpenFilesLimit().Return(256),
osMock.EXPECT().GetOpenFilesLimit().Return(256, nil),
ofMocks[1].EXPECT().Flush(),
)
},
Expand Down Expand Up @@ -236,7 +236,7 @@ func TestExportChats(t *testing.T) {
osMock.EXPECT().Create("messages-export/testdisplayname/testguid.pdf").Return(chatFile, nil),
osMock.EXPECT().NewPDFOutFile(chatFile, gomock.Any(), false).Return(ofMocks[0]),
ofMocks[0].EXPECT().Stage(),
osMock.EXPECT().GetOpenFilesLimit().Return(256),
osMock.EXPECT().GetOpenFilesLimit().Return(256, nil),
ofMocks[0].EXPECT().Flush(),
)
},
Expand Down Expand Up @@ -266,7 +266,7 @@ func TestExportChats(t *testing.T) {
osMock.EXPECT().Create("messages-export/testdisplayname/testguid.pdf").Return(chatFile, nil),
osMock.EXPECT().NewPDFOutFile(chatFile, gomock.Any(), false).Return(ofMocks[0]),
ofMocks[0].EXPECT().Stage(),
osMock.EXPECT().GetOpenFilesLimit().Return(256),
osMock.EXPECT().GetOpenFilesLimit().Return(256, nil),
ofMocks[0].EXPECT().Flush(),
)
},
Expand Down Expand Up @@ -297,7 +297,7 @@ func TestExportChats(t *testing.T) {
osMock.EXPECT().Create("messages-export/testdisplayname/testguid.txt").Return(chatFile, nil),
osMock.EXPECT().NewTxtOutFile(chatFile).Return(ofMocks[0]),
ofMocks[0].EXPECT().Stage(),
osMock.EXPECT().GetOpenFilesLimit().Return(256),
osMock.EXPECT().GetOpenFilesLimit().Return(256, nil),
ofMocks[0].EXPECT().Flush(),
)
},
Expand Down
6 changes: 5 additions & 1 deletion internal/bagoup/write.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,11 @@ func (cfg *configuration) handleFileContents(outFile opsys.OutFile, messageIDs [
if err != nil {
return errors.Wrapf(err, "stage chat file %q for writing", outFile.Name())
}
if openFilesLimit := cfg.OS.GetOpenFilesLimit(); imgCount*2 > openFilesLimit {
openFilesLimit, err := cfg.OS.GetOpenFilesLimit()
if err != nil {
return err
}
if imgCount*2 > openFilesLimit {
if err := cfg.OS.SetOpenFilesLimit(imgCount * 2); err != nil {
return errors.Wrapf(err, "chat file %q - increase the open file limit from %d to %d to support %d embedded images", outFile.Name(), openFilesLimit, imgCount*2, imgCount)
}
Expand Down
52 changes: 39 additions & 13 deletions internal/bagoup/write_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func TestWriteFile(t *testing.T) {
ofMock.EXPECT().Name().Return("messages-export/friend/iMessage;-;[email protected];;;iMessage;-;[email protected]"),
ofMock.EXPECT().ReferenceAttachment("att3transfer.png"),
ofMock.EXPECT().Stage(),
osMock.EXPECT().GetOpenFilesLimit().Return(256),
osMock.EXPECT().GetOpenFilesLimit().Return(256, nil),
ofMock.EXPECT().Flush(),
)
},
Expand All @@ -81,7 +81,7 @@ func TestWriteFile(t *testing.T) {
ofMock.EXPECT().Name().Return("messages-export/friend/iMessage;-;[email protected];;;iMessage;-;[email protected]"),
ofMock.EXPECT().ReferenceAttachment("att3transfer.png"),
ofMock.EXPECT().Stage(),
osMock.EXPECT().GetOpenFilesLimit().Return(256),
osMock.EXPECT().GetOpenFilesLimit().Return(256, nil),
ofMock.EXPECT().Flush(),
)
},
Expand Down Expand Up @@ -110,7 +110,7 @@ func TestWriteFile(t *testing.T) {
ofMock.EXPECT().Name().Return("messages-export/friend/iMessage;-;[email protected];;;iMessage;-;[email protected]"),
ofMock.EXPECT().ReferenceAttachment("att3transfer.png"),
ofMock.EXPECT().Stage().Return(500, nil),
osMock.EXPECT().GetOpenFilesLimit().Return(256),
osMock.EXPECT().GetOpenFilesLimit().Return(256, nil),
osMock.EXPECT().SetOpenFilesLimit(1000),
ofMock.EXPECT().Flush(),
)
Expand Down Expand Up @@ -140,7 +140,7 @@ func TestWriteFile(t *testing.T) {
ofMock.EXPECT().Name().Return("messages-export/friend/iMessage;-;[email protected];;;iMessage;-;[email protected]"),
ofMock.EXPECT().ReferenceAttachment("att3transfer.png"),
ofMock.EXPECT().Stage(),
osMock.EXPECT().GetOpenFilesLimit().Return(256),
osMock.EXPECT().GetOpenFilesLimit().Return(256, nil),
ofMock.EXPECT().Flush(),
)
},
Expand Down Expand Up @@ -170,7 +170,7 @@ func TestWriteFile(t *testing.T) {
ofMock.EXPECT().Name().Return("messages-export/friend/iMessage;-;[email protected];;;iMessage;-;[email protected]"),
ofMock.EXPECT().ReferenceAttachment("att3transfer.png"),
ofMock.EXPECT().Stage(),
osMock.EXPECT().GetOpenFilesLimit().Return(256),
osMock.EXPECT().GetOpenFilesLimit().Return(256, nil),
ofMock.EXPECT().Flush(),
)
},
Expand Down Expand Up @@ -274,6 +274,32 @@ func TestWriteFile(t *testing.T) {
},
wantErr: `stage chat file "messages-export/friend/iMessage;-;[email protected];;;iMessage;-;[email protected]" for writing: this is a staging error`,
},
{
msg: "get open files limit fails",
pdf: true,
setupMocks: func(dbMock *mock_chatdb.MockChatDB, osMock *mock_opsys.MockOS, ofMock *mock_opsys.MockOutFile) {
gomock.InOrder(
osMock.EXPECT().MkdirAll("messages-export/friend", os.ModePerm),
osMock.EXPECT().Create("messages-export/friend/iMessage;-;[email protected];;;iMessage;-;[email protected]").Return(chatFile, nil),
osMock.EXPECT().NewPDFOutFile(chatFile, gomock.Any(), false).Return(ofMock),
dbMock.EXPECT().GetMessage(1, nil).Return("message1", true, nil),
ofMock.EXPECT().WriteMessage("message1"),
dbMock.EXPECT().GetMessage(2, nil).Return("message2", true, nil),
ofMock.EXPECT().WriteMessage("message2"),
osMock.EXPECT().FileExist("attachment1.heic").Return(true, nil),
osMock.EXPECT().HEIC2JPG("attachment1.heic").Return("attachment1.jpeg", nil),
ofMock.EXPECT().WriteAttachment("attachment1.jpeg"),
osMock.EXPECT().FileExist("attachment2.jpeg").Return(true, nil),
osMock.EXPECT().HEIC2JPG("attachment2.jpeg").Return("attachment2.jpeg", nil),
ofMock.EXPECT().WriteAttachment("attachment2.jpeg"),
ofMock.EXPECT().Name().Return("messages-export/friend/iMessage;-;[email protected];;;iMessage;-;[email protected]"),
ofMock.EXPECT().ReferenceAttachment("att3transfer.png"),
ofMock.EXPECT().Stage().Return(500, nil),
osMock.EXPECT().GetOpenFilesLimit().Return(0, errors.New("this is a ulimit error")),
)
},
wantErr: `this is a ulimit error`,
},
{
msg: "open files limit increase fails",
pdf: true,
Expand All @@ -295,7 +321,7 @@ func TestWriteFile(t *testing.T) {
ofMock.EXPECT().Name().Return("messages-export/friend/iMessage;-;[email protected];;;iMessage;-;[email protected]"),
ofMock.EXPECT().ReferenceAttachment("att3transfer.png"),
ofMock.EXPECT().Stage().Return(500, nil),
osMock.EXPECT().GetOpenFilesLimit().Return(256),
osMock.EXPECT().GetOpenFilesLimit().Return(256, nil),
osMock.EXPECT().SetOpenFilesLimit(1000).Return(errors.New("this is a syscall error")),
ofMock.EXPECT().Name().Return("messages-export/friend/iMessage;-;[email protected];;;iMessage;-;[email protected]"),
)
Expand Down Expand Up @@ -323,7 +349,7 @@ func TestWriteFile(t *testing.T) {
ofMock.EXPECT().Name().Return("messages-export/friend/iMessage;-;[email protected];;;iMessage;-;[email protected]"),
ofMock.EXPECT().ReferenceAttachment("att3transfer.png"),
ofMock.EXPECT().Stage(),
osMock.EXPECT().GetOpenFilesLimit().Return(256),
osMock.EXPECT().GetOpenFilesLimit().Return(256, nil),
ofMock.EXPECT().Flush().Return(errors.New("this is a flush error")),
ofMock.EXPECT().Name().Return("messages-export/friend/iMessage;-;[email protected];;;iMessage;-;[email protected]"),
)
Expand All @@ -349,7 +375,7 @@ func TestWriteFile(t *testing.T) {
ofMock.EXPECT().Name().Return("messages-export/friend/iMessage;-;[email protected];;;iMessage;-;[email protected]"),
ofMock.EXPECT().ReferenceAttachment("att3transfer.png"),
ofMock.EXPECT().Stage(),
osMock.EXPECT().GetOpenFilesLimit().Return(256),
osMock.EXPECT().GetOpenFilesLimit().Return(256, nil),
ofMock.EXPECT().Flush(),
)
},
Expand Down Expand Up @@ -456,7 +482,7 @@ func TestWriteFile(t *testing.T) {
ofMock.EXPECT().Name().Return("messages-export/friend/iMessage;-;[email protected];;;iMessage;-;[email protected]"),
ofMock.EXPECT().ReferenceAttachment("att3transfer.png"),
ofMock.EXPECT().Stage(),
osMock.EXPECT().GetOpenFilesLimit().Return(256),
osMock.EXPECT().GetOpenFilesLimit().Return(256, nil),
ofMock.EXPECT().Flush(),
)
},
Expand Down Expand Up @@ -501,7 +527,7 @@ func TestWriteFile(t *testing.T) {
ofMock.EXPECT().Name().Return("messages-export/friend/iMessage;-;[email protected];;;iMessage;-;[email protected]"),
ofMock.EXPECT().ReferenceAttachment("att3transfer.png"),
ofMock.EXPECT().Stage(),
osMock.EXPECT().GetOpenFilesLimit().Return(256),
osMock.EXPECT().GetOpenFilesLimit().Return(256, nil),
ofMock.EXPECT().Flush(),
)
},
Expand Down Expand Up @@ -575,7 +601,7 @@ func TestWriteFile(t *testing.T) {
osMock.EXPECT().Create("friend/iMessage;-;heresareallylongemailaddress.heresareallylongemailaddress.heresareallylongemailaddress.heresareallylongemailaddress.heresareallylongemailaddress.heresareallylongemailaddress.heresareallylongemailaddress.heresareallylongemailaddress@gmail.c.txt").Return(chatFile, nil),
osMock.EXPECT().NewTxtOutFile(chatFile).Return(ofMock),
ofMock.EXPECT().Stage(),
osMock.EXPECT().GetOpenFilesLimit().Return(256),
osMock.EXPECT().GetOpenFilesLimit().Return(256, nil),
ofMock.EXPECT().Flush(),
)

Expand Down Expand Up @@ -617,7 +643,7 @@ func TestWriteFile(t *testing.T) {
mockCalls = append(
mockCalls,
ofMock1.EXPECT().Stage(),
osMock.EXPECT().GetOpenFilesLimit().Return(256),
osMock.EXPECT().GetOpenFilesLimit().Return(256, nil),
ofMock1.EXPECT().Flush(),
osMock.EXPECT().Create("messages-export/friend/iMessage;-;[email protected]").Return(chatFile2, nil),
osMock.EXPECT().NewPDFOutFile(chatFile2, gomock.Any(), false).Return(ofMock2),
Expand All @@ -634,7 +660,7 @@ func TestWriteFile(t *testing.T) {
mockCalls = append(
mockCalls,
ofMock2.EXPECT().Stage(),
osMock.EXPECT().GetOpenFilesLimit().Return(256),
osMock.EXPECT().GetOpenFilesLimit().Return(256, nil),
ofMock2.EXPECT().Flush(),
)
gomock.InOrder(mockCalls...)
Expand Down
5 changes: 3 additions & 2 deletions opsys/mock_opsys/mock_opsys.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

45 changes: 30 additions & 15 deletions opsys/opsys.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"os"
"os/exec"
"path/filepath"
"strconv"
"strings"
"syscall"
"unicode"
Expand Down Expand Up @@ -54,7 +55,7 @@ type (
// staging converted images for inclusion in PDF files.
RmTempDir() error
// GetOpenFilesLimit gets the current limit on the number of open files.
GetOpenFilesLimit() int
GetOpenFilesLimit() (int, error)
// SetOpenFilesLimit sets the open files limit to the given value to
// accommodate wkhtmltopdf:
// https://github.com/wkhtmltopdf/wkhtmltopdf/issues/3081#issue-172083214
Expand All @@ -80,19 +81,13 @@ type (
)

// NewOS returns an OS from a given filesystem, os Stat, and exec Command.
func NewOS(fs afero.Fs, osStat func(string) (os.FileInfo, error), sc scall.Syscall) (OS, error) {
var openFilesLimit syscall.Rlimit
if err := sc.Getrlimit(syscall.RLIMIT_NOFILE, &openFilesLimit); err != nil {
return nil, errors.Wrap(err, "check file count limit")
}
func NewOS(fs afero.Fs, osStat func(string) (os.FileInfo, error)) (OS, error) {
return &opSys{
Fs: fs,
Converter: heic2jpg.NewConverter(),
osStat: osStat,
execCommand: exec.Command,
Syscall: sc,
openFilesLimitHard: openFilesLimit.Max,
openFilesLimitSoft: int(openFilesLimit.Cur),
Fs: fs,
Converter: heic2jpg.NewConverter(),
osStat: osStat,
execCommand: exec.Command,
Syscall: scall.NewSyscall(),
}, nil
}

Expand Down Expand Up @@ -240,8 +235,28 @@ func (s *opSys) RmTempDir() error {
return nil
}

func (s opSys) GetOpenFilesLimit() int {
return int(s.openFilesLimitSoft)
func (s *opSys) GetOpenFilesLimit() (int, error) {
if s.openFilesLimitSoft > 0 {
return s.openFilesLimitSoft, nil
}
// Get the soft limit from the ulimit command because the soft limit from the
// syscall does not apply to subprocesses.
cmd := s.execCommand("ulimit", "-n")
o, err := cmd.Output()
if err != nil {
return 0, errors.Wrap(err, "call ulimit")
}
softLimit, err := strconv.Atoi(strings.TrimSuffix(string(o), "\n"))
if err != nil {
return 0, errors.Wrap(err, "parse open files soft limit")
}
s.openFilesLimitSoft = softLimit
var openFilesLimit syscall.Rlimit
if err := s.Syscall.Getrlimit(syscall.RLIMIT_NOFILE, &openFilesLimit); err != nil {
return 0, errors.Wrap(err, "get open files hard limit")
}
s.openFilesLimitHard = openFilesLimit.Max
return s.openFilesLimitSoft, nil
}

func (s *opSys) SetOpenFilesLimit(n int) error {
Expand Down
Loading

0 comments on commit b3b93a0

Please sign in to comment.