Skip to content

Commit

Permalink
Limit rate of open files (#3006)
Browse files Browse the repository at this point in the history
* Move FS stuff inside parser

* Version

* fix this
  • Loading branch information
peterebden authored Jan 2, 2024
1 parent 4e0befd commit f37243d
Show file tree
Hide file tree
Showing 13 changed files with 47 additions and 42 deletions.
4 changes: 4 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
Version 17.4.1
--------------
* Fix occasional 'too many open files' errors (#2998)

Version 17.4.0
--------------
* keep going flag (#2932)
Expand Down
2 changes: 1 addition & 1 deletion VERSION
Original file line number Diff line number Diff line change
@@ -1 +1 @@
17.4.0
17.4.1
3 changes: 2 additions & 1 deletion src/build/build_step_stress_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package build_test
import (
"fmt"
"io"
iofs "io/fs"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -105,7 +106,7 @@ func (fake *fakeParser) RegisterPreload(core.BuildLabel) error {
}

// ParseFile stub
func (fake *fakeParser) ParseFile(pkg *core.Package, label, dependent *core.BuildLabel, mode core.ParseMode, filename string) error {
func (fake *fakeParser) ParseFile(pkg *core.Package, label, dependent *core.BuildLabel, mode core.ParseMode, fs iofs.FS, filename string) error {
return nil
}

Expand Down
3 changes: 2 additions & 1 deletion src/build/build_step_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"encoding/hex"
"fmt"
"io"
iofs "io/fs"
"os"
"path/filepath"
"strings"
Expand Down Expand Up @@ -611,7 +612,7 @@ func (fake *fakeParser) RegisterPreload(core.BuildLabel) error {
}

// ParseFile stub
func (fake *fakeParser) ParseFile(pkg *core.Package, label, dependent *core.BuildLabel, mode core.ParseMode, filename string) error {
func (fake *fakeParser) ParseFile(pkg *core.Package, label, dependent *core.BuildLabel, mode core.ParseMode, fs iofs.FS, filename string) error {
return nil
}

Expand Down
2 changes: 1 addition & 1 deletion src/core/state.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ const (
// A Parser is the interface to reading and interacting with BUILD files.
type Parser interface {
// ParseFile parses a single BUILD file into the given package.
ParseFile(pkg *Package, forLabel, dependent *BuildLabel, mode ParseMode, filename string) error
ParseFile(pkg *Package, forLabel, dependent *BuildLabel, mode ParseMode, fs iofs.FS, filename string) error
// ParseReader parses a single BUILD file into the given package.
ParseReader(pkg *Package, reader io.ReadSeeker, forLabel, dependent *BuildLabel, mode ParseMode) error
// RunPreBuildFunction runs a pre-build function for a target.
Expand Down
4 changes: 2 additions & 2 deletions src/parse/asp/interpreter.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ func (i *interpreter) LoadBuiltins(filename string, contents []byte, statements
}
return i.loadBuiltinStatements(s, stmts, err)
}
stmts, err := i.parser.parse(filename)
stmts, err := i.parser.parse(nil, filename)
return i.loadBuiltinStatements(s, stmts, err)
}

Expand Down Expand Up @@ -213,7 +213,7 @@ func (i *interpreter) Subinclude(pkgScope *scope, path string, label core.BuildL
}

// If we get here, it falls to us to parse this.
stmts, err := i.parser.parse(path)
stmts, err := i.parser.parse(nil, path)
if err != nil {
panic(err) // We're already inside another interpreter, which will handle this for us.
}
Expand Down
6 changes: 3 additions & 3 deletions src/parse/asp/interpreter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func parseFileToStatementsInPkg(filename string, pkg *core.Package) (*scope, []*
panic(err)
}
parser.MustLoadBuiltins("builtins.build_defs", src)
statements, err := parser.parse(filename)
statements, err := parser.parse(nil, filename)
if err != nil {
panic(err)
}
Expand Down Expand Up @@ -570,7 +570,7 @@ func TestJSON(t *testing.T) {
panic(err)
}
parser.MustLoadBuiltins("builtins.build_defs", src)
statements, err := parser.parse("src/parse/asp/test_data/json.build")
statements, err := parser.parse(nil, "src/parse/asp/test_data/json.build")
if err != nil {
panic(err)
}
Expand Down Expand Up @@ -633,7 +633,7 @@ func TestLogConfigVariable(t *testing.T) {
panic(err)
}
parser.MustLoadBuiltins("builtins.build_defs", src)
statements, err := parser.parse("src/parse/asp/test_data/log_config.build")
statements, err := parser.parse(nil, "src/parse/asp/test_data/log_config.build")
if err != nil {
panic(err)
}
Expand Down
2 changes: 1 addition & 1 deletion src/parse/asp/logging_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func parseFile2(filename string) (*scope, error) {
panic(err)
}
parser.MustLoadBuiltins("builtins.build_defs", src)
statements, err := parser.parse(filename)
statements, err := parser.parse(nil, filename)
if err != nil {
panic(err)
}
Expand Down
2 changes: 1 addition & 1 deletion src/parse/asp/main/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func parseFile(pkg *core.Package, p *asp.Parser, filename string) error {
}
return err
}
return p.ParseFile(pkg, nil, nil, 0, filename)
return p.ParseFile(pkg, nil, nil, 0, nil, filename)
}

type assignment struct {
Expand Down
28 changes: 23 additions & 5 deletions src/parse/asp/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ package asp

import (
"bytes"
"fmt"
"io"
iofs "io/fs"
"os"
"strings"

Expand Down Expand Up @@ -71,11 +73,11 @@ func (p *Parser) MustLoadBuiltins(filename string, contents []byte) {
// ParseFile parses the contents of a single file in the BUILD language.
// It returns true if the call was deferred at some point awaiting target to build,
// along with any error encountered.
func (p *Parser) ParseFile(pkg *core.Package, label, dependent *core.BuildLabel, mode core.ParseMode, filename string) error {
func (p *Parser) ParseFile(pkg *core.Package, label, dependent *core.BuildLabel, mode core.ParseMode, fs iofs.FS, filename string) error {
p.limiter.Acquire()
defer p.limiter.Release()

statements, err := p.parse(filename)
statements, err := p.parse(fs, filename)
if err != nil {
return err
}
Expand Down Expand Up @@ -116,12 +118,12 @@ func (p *Parser) ParseReader(pkg *core.Package, r io.ReadSeeker, forLabel, depen

// ParseFileOnly parses the given file but does not interpret it.
func (p *Parser) ParseFileOnly(filename string) ([]*Statement, error) {
return p.parse(filename)
return p.parse(nil, filename)
}

// parse reads the given file and parses it into a set of statements.
func (p *Parser) parse(filename string) ([]*Statement, error) {
f, err := os.Open(filename)
func (p *Parser) parse(fs iofs.FS, filename string) ([]*Statement, error) {
f, err := p.open(fs, filename)
if err != nil {
return nil, err
}
Expand All @@ -134,6 +136,22 @@ func (p *Parser) parse(filename string) ([]*Statement, error) {
return stmts, err
}

// open opens a file from the given path
func (p *Parser) open(fs iofs.FS, filename string) (io.ReadSeekCloser, error) {
if fs == nil {
return os.Open(filename)
}
f, err := fs.Open(filename)
if err != nil {
return nil, err
}
r, ok := f.(io.ReadSeekCloser)
if !ok {
return nil, fmt.Errorf("opened file is not seekable: %s", filename)
}
return r, nil
}

// ParseData reads the given byteslice and parses it into a set of statements.
// The 'filename' argument is only used in case of errors so doesn't necessarily have to correspond to a real file.
func (p *Parser) ParseData(data []byte, filename string) ([]*Statement, error) {
Expand Down
6 changes: 3 additions & 3 deletions src/parse/asp/parser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -451,7 +451,7 @@ func TestAssert(t *testing.T) {

func TestOptimise(t *testing.T) {
p := newParser()
statements, err := p.parse("src/parse/asp/test_data/optimise.build")
statements, err := p.parse(nil, "src/parse/asp/test_data/optimise.build")
f := newFile("src/parse/asp/test_data/optimise.build")
assert.NoError(t, err)
assert.Equal(t, 5, len(statements))
Expand Down Expand Up @@ -647,12 +647,12 @@ func TestMissingNewlines(t *testing.T) {
}

func TestRepeatedArguments(t *testing.T) {
_, err := newParser().parse("src/parse/asp/test_data/repeated_arguments.build")
_, err := newParser().parse(nil, "src/parse/asp/test_data/repeated_arguments.build")
assert.Error(t, err)
}

func TestConstantAssignments(t *testing.T) {
_, err := newParser().parse("src/parse/asp/test_data/constant_assign.build")
_, err := newParser().parse(nil, "src/parse/asp/test_data/constant_assign.build")
assert.Error(t, err)
}

Expand Down
5 changes: 3 additions & 2 deletions src/parse/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package parse
import (
"fmt"
"io"
iofs "io/fs"
"os"
"path/filepath"
"sort"
Expand Down Expand Up @@ -55,8 +56,8 @@ func newAspParser(state *core.BuildState) *asp.Parser {
return p
}

func (p *aspParser) ParseFile(pkg *core.Package, forLabel, dependent *core.BuildLabel, mode core.ParseMode, filename string) error {
return p.parser.ParseFile(pkg, forLabel, dependent, mode, filename)
func (p *aspParser) ParseFile(pkg *core.Package, forLabel, dependent *core.BuildLabel, mode core.ParseMode, fs iofs.FS, filename string) error {
return p.parser.ParseFile(pkg, forLabel, dependent, mode, fs, filename)
}

func (p *aspParser) ParseReader(pkg *core.Package, reader io.ReadSeeker, forLabel, dependent *core.BuildLabel, mode core.ParseMode) error {
Expand Down
22 changes: 1 addition & 21 deletions src/parse/parse_step.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ package parse

import (
"fmt"
"io"
iofs "io/fs"
"path/filepath"
"strings"
Expand Down Expand Up @@ -158,19 +157,6 @@ func parseSubrepoPackage(state *core.BuildState, subrepoPkg, subrepoSubrepo stri
return state.CheckArchSubrepo(dependent.Subrepo), nil
}

func openFile(fs iofs.FS, subrepoName, name string) (io.ReadSeekCloser, error) {
file, err := fs.Open(name)
if err != nil {
return nil, fmt.Errorf("failed to open build file: %v", err)
}

reader, ok := file.(io.ReadSeekCloser)
if !ok {
return nil, fmt.Errorf("opened file is not seekable: ///%v/%v", subrepoName, name)
}
return reader, nil
}

// parsePackage parses a BUILD file and adds the package to the build graph
func parsePackage(state *core.BuildState, label, dependent core.BuildLabel, subrepo *core.Subrepo, mode core.ParseMode) (*core.Package, error) {
packageName := label.PackageName
Expand All @@ -193,14 +179,8 @@ func parsePackage(state *core.BuildState, label, dependent core.BuildLabel, subr
} else {
filename, dir := buildFileName(state, subrepo, fileSystem, label.PackageName)
if filename != "" {
file, err := openFile(fileSystem, pkg.SubrepoName, filename)
if err != nil {
return nil, err
}
defer file.Close()

pkg.Filename = filename
if err := state.Parser.ParseReader(pkg, file, &label, &dependent, mode); err != nil {
if err := state.Parser.ParseFile(pkg, &label, &dependent, mode, fileSystem, filename); err != nil {
return nil, err
}
} else {
Expand Down

0 comments on commit f37243d

Please sign in to comment.