Skip to content

Commit 79c4711

Browse files
ZNeumannZNeumann
committed
improve valgrind options
1 parent f2c18ed commit 79c4711

File tree

5 files changed

+162
-29
lines changed

5 files changed

+162
-29
lines changed

‎.github/workflows/make-integration-tests.yml

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,10 @@ on:
2424
description: 'runner type'
2525
required: true
2626
type: string
27+
use-valgrind:
28+
required: true
29+
default: false
30+
type: boolean
2731

2832
jobs:
2933
test-agent:
@@ -77,7 +81,7 @@ jobs:
7781
run: |
7882
docker exec \
7983
-e PHPS=${{matrix.php}} \
80-
-e INTEGRATION_ARGS="-license ${{secrets.NR_TEST_LICENSE}} -collector ${{secrets.NR_COLLECTOR_HOST}} -agent agent/modules/newrelic.so" \
84+
-e INTEGRATION_ARGS="-license ${{secrets.NR_TEST_LICENSE}} -collector ${{secrets.NR_COLLECTOR_HOST}} -agent agent/modules/newrelic.so ${{ inputs.arch != 'arm64' && inputs.use-valgrind && '-valgrind /usr/bin/valgrind' }}" \
8185
-e APP_supportability=${{secrets.APP_SUPPORTABILITY}} \
8286
-e ACCOUNT_supportability=${{secrets.ACCOUNT_SUPPORTABILITY}} \
8387
-e ACCOUNT_supportability_trusted=${{secrets.ACCOUNT_SUPPORTABILITY_TRUSTED}} \

‎.github/workflows/run-integration-tests.yml

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,11 @@ on:
2626
options:
2727
- ubuntu-22.04-arm
2828
- ubuntu-24.04-arm
29-
29+
use-valgrind:
30+
description: 'Use valgrind?'
31+
required: true
32+
default: false
33+
type: boolean
3034
jobs:
3135
build-test-runner-amd64:
3236
uses: ./.github/workflows/make-for-platform-on-arch.yml
@@ -78,6 +82,7 @@ jobs:
7882
with:
7983
origin: ${{ inputs.origin }}
8084
ref: ${{ inputs.ref }}
85+
use-valgrind: ${{ inputs.use-valgrind }}
8186
arch: amd64
8287
runs-on: ubuntu-latest
8388
continue-on-error: false
@@ -88,6 +93,7 @@ jobs:
8893
with:
8994
origin: ${{ inputs.origin }}
9095
ref: ${{ inputs.ref }}
96+
use-valgrind: ${{ inputs.use-valgrind }}
9197
arch: arm64
9298
runs-on: ${{inputs.runner-type}}
9399
continue-on-error: true

‎daemon/cmd/integration_runner/main.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -344,6 +344,10 @@ func main() {
344344

345345
ctx = integration.NewContext(*flagPHP, *flagCGI)
346346
ctx.Valgrind = *flagValgrind
347+
if (ctx.Valgrind != "" && *flagWorkers != 1) {
348+
fmt.Fprintf(os.Stderr, "cannot run valgrind on multiple workers\n", err)
349+
os.Exit(1)
350+
}
347351
ctx.Timeout = *flagTimeout
348352

349353
// Settings common to all tests.

‎daemon/internal/newrelic/integration/transaction.go

Lines changed: 61 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ func PhpTx(src Script, env, settings map[string]string, ctx *Context) (Tx, error
4848

4949
args := phpArgs(nil, filepath.Base(src.Name()), false, settings)
5050

51-
if ctx.Valgrind != "" {
51+
if ctx.Valgrind != "" && settings["newrelic.appname"] != "skipif" {
5252
txn = &ValgrindCLI{
5353
CLI: CLI{
5454
Path: ctx.PHP,
@@ -81,6 +81,7 @@ func PhpTx(src Script, env, settings map[string]string, ctx *Context) (Tx, error
8181
// See: https://tools.ietf.org/html/rfc3875
8282
func CgiTx(src Script, env, settings map[string]string, headers http.Header, ctx *Context) (Tx, error) {
8383
var err error
84+
var txn Tx
8485

8586
req := &http.Request{
8687
Method: env["REQUEST_METHOD"],
@@ -105,34 +106,69 @@ func CgiTx(src Script, env, settings map[string]string, headers http.Header, ctx
105106
return nil, fmt.Errorf("unable to create cgi request: %v", err)
106107
}
107108

108-
tx := &CGI{
109-
request: req,
110-
handler: &cgi.Handler{
111-
Path: ctx.CGI,
112-
Dir: src.Dir(),
113-
Args: phpArgs(nil, "", false, settings),
114-
},
115-
}
109+
if ctx.Valgrind != "" {
110+
tx := &ValgrindCGI{
111+
CGI: CGI{
112+
request: req,
113+
handler: &cgi.Handler{
114+
Path: ctx.CGI,
115+
Dir: src.Dir(),
116+
Args: phpArgs(nil, "", false, settings),
117+
},
118+
},
119+
Valgrind: ctx.Valgrind,
120+
Timeout: 60*time.Second,
121+
}
122+
tx.handler.Env = append(tx.handler.Env,
123+
"SCRIPT_FILENAME="+scriptFile,
124+
"SCRIPT_NAME=/"+filepath.Base(src.Name()),
125+
"REDIRECT_STATUS=200")
126+
tx.handler.Env = append(tx.handler.Env, flatten(env)...)
127+
128+
// If the environment includes a REQUEST_URI and doesn't include an explicit
129+
// PATH_INFO, we'll set it here to avoid problems with Go's CGI package
130+
// attempting to guess what the PATH_INFO should be.
131+
if ruri, ok := env["REQUEST_URI"]; ok {
132+
if _, ok := env["PATH_INFO"]; !ok {
133+
tx.handler.Env = append(tx.handler.Env, "PATH_INFO="+ruri)
134+
}
135+
}
116136

117-
tx.handler.Env = append(tx.handler.Env,
118-
"SCRIPT_FILENAME="+scriptFile,
119-
"SCRIPT_NAME=/"+filepath.Base(src.Name()),
120-
"REDIRECT_STATUS=200")
121-
tx.handler.Env = append(tx.handler.Env, flatten(env)...)
122-
123-
// If the environment includes a REQUEST_URI and doesn't include an explicit
124-
// PATH_INFO, we'll set it here to avoid problems with Go's CGI package
125-
// attempting to guess what the PATH_INFO should be.
126-
if ruri, ok := env["REQUEST_URI"]; ok {
127-
if _, ok := env["PATH_INFO"]; !ok {
128-
tx.handler.Env = append(tx.handler.Env, "PATH_INFO="+ruri)
137+
if !src.IsFile() {
138+
return withTempFile(tx, src.Name(), src.Bytes()), nil
139+
}
140+
txn = tx
141+
} else {
142+
tx := &CGI{
143+
request: req,
144+
handler: &cgi.Handler{
145+
Path: ctx.CGI,
146+
Dir: src.Dir(),
147+
Args: phpArgs(nil, "", false, settings),
148+
},
149+
}
150+
tx.handler.Env = append(tx.handler.Env,
151+
"SCRIPT_FILENAME="+scriptFile,
152+
"SCRIPT_NAME=/"+filepath.Base(src.Name()),
153+
"REDIRECT_STATUS=200")
154+
tx.handler.Env = append(tx.handler.Env, flatten(env)...)
155+
156+
// If the environment includes a REQUEST_URI and doesn't include an explicit
157+
// PATH_INFO, we'll set it here to avoid problems with Go's CGI package
158+
// attempting to guess what the PATH_INFO should be.
159+
if ruri, ok := env["REQUEST_URI"]; ok {
160+
if _, ok := env["PATH_INFO"]; !ok {
161+
tx.handler.Env = append(tx.handler.Env, "PATH_INFO="+ruri)
162+
}
129163
}
130-
}
131164

132-
if !src.IsFile() {
133-
return withTempFile(tx, src.Name(), src.Bytes()), nil
165+
if !src.IsFile() {
166+
return withTempFile(tx, src.Name(), src.Bytes()), nil
167+
}
168+
txn = tx
134169
}
135-
return tx, nil
170+
171+
return txn, nil
136172
}
137173

138174
// phpArgs builds the command line for a PHP process.

‎daemon/internal/newrelic/integration/valgrind.go

Lines changed: 85 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"fmt"
1111
"io/ioutil"
1212
"net/http"
13+
"net/http/httptest"
1314
"sync"
1415
"time"
1516

@@ -24,6 +25,12 @@ type ValgrindCLI struct {
2425
Timeout time.Duration
2526
}
2627

28+
type ValgrindCGI struct {
29+
CGI
30+
Valgrind string
31+
Timeout time.Duration
32+
}
33+
2734
func (tx *ValgrindCLI) Execute() (http.Header, []byte, error) {
2835
if len(tx.Path) == 0 {
2936
return nil, []byte("skip: executable not specified"), nil
@@ -34,8 +41,8 @@ func (tx *ValgrindCLI) Execute() (http.Header, []byte, error) {
3441
// valgrind reports to their tests in the same way we use the appname
3542
// to link the transaction data. Failing that, perhaps we could use
3643
// the valgrind process's pid instead.
37-
valgrindMu.Lock()
38-
defer valgrindMu.Unlock()
44+
//valgrindMu.Lock()
45+
//defer valgrindMu.Unlock()
3946

4047
cmd := valgrind.Memcheck(tx.Valgrind, "--quiet")
4148
cmd.Args = append(cmd.Args, "--xml=yes")
@@ -105,6 +112,82 @@ func (tx *ValgrindCLI) Execute() (http.Header, []byte, error) {
105112
return nil, output, err
106113
}
107114

115+
func (tx *ValgrindCGI) Execute() (http.Header, []byte, error) {
116+
if len(tx.handler.Path) == 0 {
117+
return nil, []byte("skip: executable not specified"), nil
118+
}
119+
120+
// For now, we don't have a mechanism to handle concurrent invocations
121+
// of Valgrind. In the future, we could use the appname to connect
122+
// valgrind reports to their tests in the same way we use the appname
123+
// to link the transaction data. Failing that, perhaps we could use
124+
// the valgrind process's pid instead.
125+
//valgrindMu.Lock()
126+
//defer valgrindMu.Unlock()
127+
128+
cmd := valgrind.Memcheck(tx.Valgrind, "--quiet")
129+
cmd.Args = append(cmd.Args, "--xml=yes")
130+
//cmd.Args = append(cmd.Args, "--child-silent-after-fork=no")
131+
cmd.Args = append(cmd.Args, "--xml-socket="+valgrindLn.Addr().String())
132+
cmd.Args = append(cmd.Args, "--")
133+
cmd.Args = append(cmd.Args, tx.handler.Path)
134+
if len(tx.handler.Args) > 0 {
135+
cmd.Args = append(cmd.Args, tx.handler.Args...)
136+
}
137+
138+
log.Debugf("command: %v", cmd)
139+
140+
// Replace the handler with valgrind
141+
tx.handler.Path = cmd.Path
142+
tx.handler.Args = cmd.Args[1:] // The first Arg is the Path.
143+
// ServeHTTP will re-add Path
144+
// to the front of Args
145+
146+
ch := make(chan resultOrError, 1)
147+
go func() {
148+
var result resultOrError
149+
result.R, result.E = acceptOneReport(tx.Timeout)
150+
ch <- result
151+
}()
152+
153+
resp := httptest.NewRecorder()
154+
tx.handler.ServeHTTP(resp, tx.request)
155+
156+
vgOutput := <-ch
157+
158+
// Append the output from Valgrind to the test output.
159+
//
160+
// TODO: Eventually we want to report valgrind output separately, so we can
161+
// treat valgrind errors similarly to failed test expectations. i.e. We'd
162+
// like to add them to Test.Failures. After all, each test has an implicit
163+
// expectation that it will not exhibit memory bugs!
164+
//
165+
// TODO: Once the former is in place, we should be able to parse the memory
166+
// leak reports produced by the Zend Memory Manager from the test output and
167+
// report them the same way as valgrind errors.
168+
output := resp.Body.Bytes()
169+
if vgOutput.R != nil && len(vgOutput.R.Errors) > 0 {
170+
// Safe to ignore the error here, Report.MarshalText() never fails.
171+
data, _ := vgOutput.R.MarshalText()
172+
output = append(output, '\n')
173+
output = append(output, data...)
174+
}
175+
176+
177+
// Ensure a non-nil error is returned when valgrind detects errors.
178+
// Otherwise, the test could be marked as passing if it does not have
179+
// any expectations on the test output. This sucks.
180+
//
181+
// TODO: Remove this when valgrind errors can be treated as failed test
182+
// expectations.
183+
err := vgOutput.E
184+
if err == nil && vgOutput.R != nil && len(vgOutput.R.Errors) > 0 {
185+
err = fmt.Errorf("detected %d memory errors", len(vgOutput.R.Errors))
186+
}
187+
188+
return resp.HeaderMap, output, err
189+
190+
}
108191
// resultOrError is a poor man's sum type.
109192
type resultOrError struct {
110193
R *valgrind.Report

0 commit comments

Comments
 (0)