scope tracker: attach scope trackers to Http:Client and PlatformBridgeFilter#1498
scope tracker: attach scope trackers to Http:Client and PlatformBridgeFilter#1498
Conversation
…eFilter Signed-off-by: Jose Nino <jnino@lyft.com>
library/common/extensions/filters/http/platform_bridge/filter.cc
Outdated
Show resolved
Hide resolved
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
library/common/extensions/filters/http/platform_bridge/filter.cc
Outdated
Show resolved
Hide resolved
library/common/extensions/filters/http/platform_bridge/filter.cc
Outdated
Show resolved
Hide resolved
library/common/extensions/filters/http/platform_bridge/filter.h
Outdated
Show resolved
Hide resolved
|
note: this PR depends on envoyproxy/envoy#16709 and envoyproxy/envoy#16738 |
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
| Http::TestResponseTrailerMapImpl response_trailers{{"x-test-trailer", "test trailer"}}; | ||
| EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->encodeTrailers(response_trailers)); | ||
|
|
||
| checkFilterState("NullImplementation", "0", |
There was a problem hiding this comment.
Will add throughout the rest of the test suite once we are finalized on all the state we want to keep track of and dump.
| } | ||
|
|
||
| void ProvisionalDispatcher::pushTrackedObject(const ScopeTrackedObject* object) { | ||
| if (event_dispatcher_) { |
There was a problem hiding this comment.
This access is not inherently threadsafe, though it will likely only be called from a valid context. Still, we might want to have a safety check in these calls (like we do with deferredDelete).
There was a problem hiding this comment.
currently scopes are only created in the threading context of the event dispatcher. I removed all if statements and put in asserts like in deferred delete
| } | ||
| } | ||
|
|
||
| bool ProvisionalDispatcher::trackedObjectStackIsEmpty() const { |
There was a problem hiding this comment.
Based on your check below, we might want to lock here, depending on the usage.
There was a problem hiding this comment.
This actually never gets currently called. I could change usage to NOT_REACHED. But for now I changed to isThreadSafe assertion.
|
Overall looks good, just some concerns around threadsafety when accessing the underlying dispatcher. |
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
| EXPECT_TRUE(response->complete()); | ||
| EXPECT_EQ("200", response->headers().Status()->value().getStringView()); | ||
| } | ||
| // TEST_P(PlatformBridgeIntegrationTest, MultipleFilters) { |
There was a problem hiding this comment.
@goaway given how request and responses are passed in threading contexts in the integration text framework, I am having trouble getting the filters to run in the thread of their own dispatcher without stalling the test.
If you are ok with it, could we maybe land this PR, and I can untangle the threading issue in this integration test in a subsequent PR?
There was a problem hiding this comment.
I can open a ticket to track
There was a problem hiding this comment.
Yes, please open a tracking issue/ticket.
|
|
||
| // Encode response headers. | ||
| EXPECT_CALL(dispatcher_, pushTrackedObject(_)); | ||
| EXPECT_CALL(dispatcher_, popTrackedObject(_)); |
There was a problem hiding this comment.
Don't need to block on this, but we might just want to adopt NiceMock behavior for these calls so we don't have to count them everywhere in tests.
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
|
Nice work! |
2 participants
Description: scope tracker - attach scope trackers to Http:Client and PlatformBridgeFilter
Risk Level: med - changing crash clean up routine
Testing: example app. Integration test pending.
Signed-off-by: Jose Nino jnino@lyft.com