Skip to content
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

test: fix data races in FakeStream. #7929

Merged
merged 3 commits into from
Aug 19, 2019
Merged

test: fix data races in FakeStream. #7929

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
555b7a7
test: fix data races in FakeStream.
PiotrSikora Aug 15, 2019
eafd564
Merge remote-tracking branch 'origin/master' into PiotrSikora/fix-fak…
PiotrSikora Aug 18, 2019
ceb77ef
review: add comments about std::shared.
PiotrSikora Aug 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 21 additions & 13 deletions test/integration/fake_upstream.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,22 +73,26 @@ void FakeStream::decodeMetadata(Http::MetadataMapPtr&& metadata_map_ptr) {
}

void FakeStream::encode100ContinueHeaders(const Http::HeaderMapImpl& headers) {
std::shared_ptr<Http::HeaderMapImpl> headers_copy(
std::unique_ptr<Http::HeaderMapImpl> headers_copy(
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment here and below on why we are doing this dance? It seems suboptimal and I'm unsure why the old code didn't work with shared_ptr correctly handling deletion at the right time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What comment would you like to see, exactly?

Copy link
Member

Choose a reason for hiding this comment

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

Why it is that shared_ptr does not work. It's not clear to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not thread-safe, but I don't know why either (...or whether it's expected to be, to be honest).

@lizan any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

shard_ptr is partially thread-safe, it does guard on the refcount part but not the holding object. But TBH I couldn't explain why exactly this case flags TSAN with libc++. I think we're in one of the not-well supported case listed here, statically linking lib(std)c++, and not using instrumented libc++.

An alternative way to do this is use the copyable TestHeaderMapImpl and forget about smart pointers for the sake of simplicity. This path is not performance critical anyway so I guess that's fine too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@silentdai that code doesn't fix the issue.

@mattklein123 how do you want to proceed?

Copy link
Member

Choose a reason for hiding this comment

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

@silentdai I tried it before commenting #7927, as I said it is likely a false positive of TSAN with non instrumented stdlib so we're not fixing anything really

Copy link
Member

Choose a reason for hiding this comment

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

@PiotrSikora this "fix" doesn't make any sense to me, so it must be a bug in the sanitizer. I'm fine doing this, especially in test code, but can you put a comment that links back to this issue and perhaps says that it is probably a bug in the fuzzer, etc.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is, there is enough articles about and hacks for thread-safety of std::shared_ptr passed to lambda that I'm not convinced that the old code is supposed to work.

I've added neutral comment about the issue, let me know if it works for you.

Copy link
Member

Choose a reason for hiding this comment

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

As long as the shared_ptr is being copied, and not captured by reference, this should be safe, and must be a bug in either the compiler, std library, or the fuzzer. I will take a look at the comment.

new Http::HeaderMapImpl(static_cast<const Http::HeaderMap&>(headers)));
parent_.connection().dispatcher().post(
[this, headers_copy]() -> void { encoder_.encode100ContinueHeaders(*headers_copy); });
parent_.connection().dispatcher().post([this, headers = headers_copy.release()]() -> void {
encoder_.encode100ContinueHeaders(*headers);
PiotrSikora marked this conversation as resolved.
Show resolved Hide resolved
delete headers;
});
}

void FakeStream::encodeHeaders(const Http::HeaderMapImpl& headers, bool end_stream) {
std::shared_ptr<Http::HeaderMapImpl> headers_copy(
std::unique_ptr<Http::HeaderMapImpl> headers_copy(
new Http::HeaderMapImpl(static_cast<const Http::HeaderMap&>(headers)));
if (add_served_by_header_) {
headers_copy->addCopy(Http::LowerCaseString("x-served-by"),
parent_.connection().localAddress()->asString());
}
parent_.connection().dispatcher().post([this, headers_copy, end_stream]() -> void {
encoder_.encodeHeaders(*headers_copy, end_stream);
});
parent_.connection().dispatcher().post(
[this, headers = headers_copy.release(), end_stream]() -> void {
encoder_.encodeHeaders(*headers, end_stream);
delete headers;
});
}

void FakeStream::encodeData(absl::string_view data, bool end_stream) {
Expand All @@ -106,16 +110,20 @@ void FakeStream::encodeData(uint64_t size, bool end_stream) {
}

void FakeStream::encodeData(Buffer::Instance& data, bool end_stream) {
std::shared_ptr<Buffer::Instance> data_copy(new Buffer::OwnedImpl(data));
parent_.connection().dispatcher().post(
[this, data_copy, end_stream]() -> void { encoder_.encodeData(*data_copy, end_stream); });
std::unique_ptr<Buffer::Instance> data_copy(new Buffer::OwnedImpl(data));
parent_.connection().dispatcher().post([this, data = data_copy.release(), end_stream]() -> void {
encoder_.encodeData(*data, end_stream);
delete data;
});
}

void FakeStream::encodeTrailers(const Http::HeaderMapImpl& trailers) {
std::shared_ptr<Http::HeaderMapImpl> trailers_copy(
std::unique_ptr<Http::HeaderMapImpl> trailers_copy(
new Http::HeaderMapImpl(static_cast<const Http::HeaderMap&>(trailers)));
parent_.connection().dispatcher().post(
[this, trailers_copy]() -> void { encoder_.encodeTrailers(*trailers_copy); });
parent_.connection().dispatcher().post([this, trailers = trailers_copy.release()]() -> void {
encoder_.encodeTrailers(*trailers);
delete trailers;
});
}

void FakeStream::encodeResetStream() {
Expand Down