Skip to content

http: fix http/1 client low watermark crash #10657

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Apr 7, 2020
Merged

http: fix http/1 client low watermark crash #10657

merged 5 commits into from
Apr 7, 2020

Conversation

mattklein123
Copy link
Member

@mattklein123 mattklein123 commented Apr 5, 2020 •
edited
Loading

This is a regression from #10561.

Fixes #10655

Risk Level: Low (for this fix, the original change was/is still medium)
Testing: New UT
Docs Changes: N/A
Release Notes: N/A

@mattklein123
http: fix http/1 client low watermark crash
46f17a1 
This is a regression from #10566.

Fixes #10655

Signed-off-by: Matt Klein <mklein@lyft.com>
@mattklein123
Copy link
Member Author

cc @euroelessar @rgs1

@rgs1
Copy link
Member

rgs1 commented Apr 5, 2020

@mattklein123 thanks! I'll give it a spin with real traffic tomorrow (but if you want to merge before, that's good too).

@mattklein123
Copy link
Member Author

@htuch @asraa I've re-run TSAN 3 times and it keeps failing on:

[2020-04-06 02:23:47.923][156][critical][assert] [source/common/event/file_event_impl.cc:65] assert failure: events.
[2020-04-06 02:23:47.923][156][critical][backtrace] [bazel-out/k8-dbg/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:104] Caught Aborted, suspect faulting address 0xfffe00000011
[2020-04-06 02:23:47.923][156][critical][backtrace] [bazel-out/k8-dbg/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:91] Backtrace (use tools/stack_decode.py to get line numbers):
[2020-04-06 02:23:47.923][156][critical][backtrace] [bazel-out/k8-dbg/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:92] Envoy version: 0/1.14.0-dev/redacted/DEBUG/BoringSSL
[2020-04-06 02:23:47.943][150][info][main] [source/server/server.cc:613] shutting down server instance
[2020-04-06 02:23:47.946][150][info][main] [source/server/server.cc:560] main dispatch loop exited
[2020-04-06 02:23:48.031][156][critical][backtrace] [bazel-out/k8-dbg/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #0: Envoy::SignalAction::sigHandler() [0x5a8e817]
[2020-04-06 02:23:48.132][156][critical][backtrace] [bazel-out/k8-dbg/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:96] #1: __tsan::CallUserSignalHandler() [0x2684350]
external/bazel_tools/tools/test/test-setup.sh: line 310:    17 Aborted                 (core dumped) "${TEST_PATH}" "$@" 2>&1
================================================================================
FAIL: //test/integration:h1_capture_fuzz_test (see /build/tmp/_bazel_bazel/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy/bazel-out/k8-dbg/testlogs/test/integration/h1_capture_fuzz_test/test.log)
INFO: From Testing //test/integration:h1_capture_fuzz_test:

I tried running this locally in TSAN mode and it passed. I'm at a loss for how my change would effect this test in this way either way. Any ideas here?

@mattklein123
Copy link
Member Author

Nevermind I did get this to reproduce locally with a non-RBE build. Looking.

@mattklein123
Copy link
Member Author

Hmm, I think I'm hitting libevent/libevent#984 and #8199, but I don't understand how my patch would effect this. Also, it seems to fail sporadically so maybe tickling a timing issue. @htuch any suggestions?

@alyssawilk
Copy link
Contributor

You could try flake instructions before and after and see if it's statistically worse?

@mattklein123
Merge remote-tracking branch 'origin/master' into fix_h1_crash
8b7d116 
Signed-off-by: Matt Klein <mklein@lyft.com>
@mattklein123
Merge remote-tracking branch 'origin/master' into fix_h1_crash
c171e83 
Signed-off-by: Matt Klein <mklein@lyft.com>
@mattklein123
Copy link
Member Author

@alyssawilk updated w/ merged flake fix.

alyssawilk
alyssawilk approved these changes Apr 6, 2020
View reviewed changes
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this and thanks even more for the "we need to clean this up" comment because we really need to clean that up :-/

@mattklein123
Merge remote-tracking branch 'origin/master' into fix_h1_crash
f101359 
Signed-off-by: Matt Klein <mklein@lyft.com>
@mattklein123
Merge remote-tracking branch 'origin/master' into fix_h1_crash
53ee1c5 
Signed-off-by: Matt Klein <mklein@lyft.com>
@mattklein123 mattklein123 merged commit 0b4381c into master Apr 7, 2020
@mattklein123 mattklein123 deleted the fix_h1_crash branch April 7, 2020 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@alyssawilk alyssawilk alyssawilk approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@alyssawilk alyssawilk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Crash since 3e1806f1e12ec
3 participants
@mattklein123 @rgs1 @alyssawilk