Enabled heap check (#1306) by mrice32 · Pull Request #1316 · envoyproxy/envoy

mrice32 · 2017-07-24T16:11:16Z

Fixes #1306. Heap check was disabled because the environment variables that enable it were not accessible to the running tests. This was fixed by explicitly adding those environment variables to the bazel test options. We now see stack traces for leaks like the following (symbol name availability depends on the build mode):

Leak of 8 bytes in 2 objects allocated from:
        @ 677c96 Envoy::Router::Filter::Filter
        @ 677b07 _ZN5Envoy6Router10ProdFilterCI2NS0_6FilterEERNS0_12FilterConfigE
        @ 958ec0 Envoy::Http::AsyncStreamImpl::AsyncStreamImpl
        @ 958757 Envoy::Http::AsyncRequestImpl::AsyncRequestImpl
        @ 958562 Envoy::Http::AsyncClientImpl::send
        @ 99f595 Envoy::Http::RestApiFetcher::refresh
        @ 99f3f5 Envoy::Http::RestApiFetcher::initialize
        @ 5885b9 Envoy::Upstream::CdsApiImpl::initialize
        @ 5667b9 Envoy::Upstream::ClusterManagerInitHelper::maybeFinishInitialize
        @ 566891 Envoy::Upstream::ClusterManagerInitHelper::onSt
Leak of 8 bytes in 2 objects allocated from:
        @ 677d38 Envoy::Router::Filter::Filter
        @ 677b07 _ZN5Envoy6Router10ProdFilterCI2NS0_6FilterEERNS0_12FilterConfigE
        @ 958ec0 Envoy::Http::AsyncStreamImpl::AsyncStreamImpl
        @ 958757 Envoy::Http::AsyncRequestImpl::AsyncRequestImpl
        @ 958562 Envoy::Http::AsyncClientImpl::send
        @ 99f595 Envoy::Http::RestApiFetcher::refresh
        @ 99f3f5 Envoy::Http::RestApiFetcher::initialize
        @ 5885b9 Envoy::Upstream::CdsApiImpl::initialize
        @ 5667b9 Envoy::Upstream::ClusterManagerInitHelper::maybeFinishInitialize
        @ 566891 Envoy::Upstream::ClusterManagerInitHelper::onSt

mattklein123

Sweet. Thanks @mrice32 !

dnoe

Thanks for figuring this out

htuch · 2017-07-24T17:10:43Z

I think we should probably put this in tools/bazel.rc so it's picked up on non-Docker build as well.

mattklein123 · 2017-07-24T17:11:24Z

Oh yeah putting in bazel.rc would be great. Good call.

htuch · 2017-07-24T17:38:39Z

ci/build_setup.sh

  --jobs=${NUM_CPUS} --show_task_finish"
 export BAZEL_TEST_OPTIONS="${BAZEL_BUILD_OPTIONS} --test_env=HOME --test_env=PYTHONUSERBASE \
-  --test_env=HEAPCHECK --test_env=PPROF_PATH --cache_test_results=no --test_output=all"
+  --test_env=HEAPCHECK --test_output=all"


I think you were off-by-one in arg position in the cut+paste here..

htuch · 2017-07-24T17:40:30Z

tools/bazel.rc

 build:clang-msan --copt -fsanitize-memory-track-origins=2
+
+# Test options
+test --test_env=HEAPCHECK --test_env=PPROF_PATH


Should this be --test_env=HEAPCHECK=normal? Is there a way to get the pprof location from the gperftools build in the local situation? Or if it's too complicated, what happens when you specify HEAPCHECK=normal but no PPROF_PATH?

I think setting HEAPCHECK to normal in the bazel.rc is preferable. I'll do some testing. I think PPROF_PATH defaults to pprof (so it should find it if you have it installed). I'm not sure what happens if pprof doesn't exist. HEAPCHECK should work okay without using pprof anyway. It apparently just makes the heap check better.

Nothing breaks if pprof isn't installed, and the heap checker still runs. Setting --test_env=HEAPCHECK=normal --test_env=PPROF_PATH should be fine in the bazel.rc. Note: If HEAPCHECK is explicitly set to normal in bazel.rc the user cannot override it by just exporting HEAPCHECK in their local environment. The user will have to explicitly set --test_env=HEAPCHECK (grabbing the env variable) or --test_env=HEAPCHECK=[user_setting] (modifying it in the command directly) to override the normal setting. Whatever we choose, I'll add it into the docs.

I don't have a strong opinion on which one is chosen as long as we doc it. Will let both of you choose.

mrice32 · 2017-07-24T17:40:42Z

To be clear, putting this in bazel.rc will just import the environment variables. It will not set them. So to enable the heap check, the user will need to export HEAPCHECK=normal.

htuch · 2017-07-24T17:41:30Z

Can you add a section to bazel/README.md on how to setup the env and do a heap check?

Description: in general java String usage accross the JNI layer is precarious. Reverting usage back to ByteBuffer. Risk Level: low Testing: tested with modified dispatcher code in example app that accessed the installed string accessor in MainActivity.kt Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

**Description** Before, there was no feedback at all unless you turn on `--debug`. Choosing what to enable is difficult as the EG layer creates known errors that confuse, and our layer we log too much debug as info. This buys time by making a simple status message and verifying it in tests. .e.g. ```bash $ out/aigw-darwin-arm64 run --mcp-json '{ "mcpServers": { "kiwi": { "type": "http", "url": "https://mcp.kiwi.com" } } }' Envoy AI Gateway listening on http://localhost:1975 (admin http://localhost:51333) after 16.8s ``` Note, it most importantly only shows when (significantly later) envoy is actually ready. If you send requests before this, it will fail. Future PRs can improve logging. **Related Issues/PRs (if applicable)** Fixes #1304 --------- Signed-off-by: Adrian Cole <adrian@tetrate.io>

Added environment variables to test options

dc2348d

mattklein123 previously approved these changes Jul 24, 2017

View reviewed changes

dnoe previously approved these changes Jul 24, 2017

View reviewed changes

Moved arguments from ci to bazel.rc

b1a3ebf

mrice32 dismissed stale reviews from dnoe and mattklein123 via b1a3ebf July 24, 2017 17:37

Fixed typo

647a691

htuch reviewed Jul 24, 2017

View reviewed changes

Added heapchecker default setting to bazel rc and docs

1f53ecd

htuch approved these changes Jul 24, 2017

View reviewed changes

mattklein123 approved these changes Jul 24, 2017

View reviewed changes

mattklein123 merged commit b1e139b into envoyproxy:master Jul 24, 2017

mrice32 deleted the heap_check_fix branch July 24, 2017 19:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabled heap check (#1306)#1316

Enabled heap check (#1306)#1316
mattklein123 merged 4 commits intoenvoyproxy:masterfrom
mrice32:heap_check_fix

mrice32 commented Jul 24, 2017

Uh oh!

mattklein123 left a comment

Uh oh!

dnoe left a comment

Uh oh!

htuch commented Jul 24, 2017

Uh oh!

mattklein123 commented Jul 24, 2017

Uh oh!

htuch Jul 24, 2017

Uh oh!

htuch Jul 24, 2017

Uh oh!

mrice32 Jul 24, 2017 •

edited

Loading

Uh oh!

mrice32 Jul 24, 2017

Uh oh!

mattklein123 Jul 24, 2017

Uh oh!

mrice32 commented Jul 24, 2017

Uh oh!

htuch commented Jul 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

mrice32 commented Jul 24, 2017

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

dnoe left a comment

Choose a reason for hiding this comment

Uh oh!

htuch commented Jul 24, 2017

Uh oh!

mattklein123 commented Jul 24, 2017

Uh oh!

htuch Jul 24, 2017

Choose a reason for hiding this comment

Uh oh!

htuch Jul 24, 2017

Choose a reason for hiding this comment

Uh oh!

mrice32 Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrice32 Jul 24, 2017

Choose a reason for hiding this comment

Uh oh!

mattklein123 Jul 24, 2017

Choose a reason for hiding this comment

Uh oh!

mrice32 commented Jul 24, 2017

Uh oh!

htuch commented Jul 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mrice32 Jul 24, 2017 •

edited

Loading