Server-side AsyncContext initialized in lifecycle observer is lost
#3111
+574
−208
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Motivation: If users put any data into `AsyncContext` inside `HttpLifecycleObserver` configured via `HttpServerBuilder.lifecycleObserver(...)` it won't be visible for filters and service because `ClearAsyncContextHttpServiceFilter` is appended after `HttpLifecycleObserverServiceFilter` inside `applyInternalFilters`. Modifications: 1. Move `ClearAsyncContextHttpServiceFilter` from the beginning of `noOffloadServiceFilters` to `applyInternalFilters`. However, this move discovered another bug in path that handles `noOffloadServiceFilters.isEmpty()` case because it never adds `OffloadingFilter`. 2. Refactor `listenForService` method to always make a copy of filters lists, then prepend/append internal filters to those copies in easy to read/understand way, and construct a single filters `Stream` for `buildService` method. 3. Refactor `OffloadingFilter` to act as a regular filter factory instead of taking all further `serviceFilters` as pre-built factory. 4. Rename `ClearAsyncContextHttpServiceFilter` singleton instance. 5. Fix another bug when `OptionalSslNegotiator` binder takes a raw service instead of `filteredService` (user-defined filters were never applied for this path). 6. Modify `AbstractHttpServiceAsyncContextTest` to test `AsyncContext` propagation from lifecycle observer and non-offloading filters. 7. Modify `AbstractHttpServiceAsyncContextTest` to test `AsyncContext` initialized by early/late connection acceptor is not visible at request path. 8. Add `BlockingHttpServiceAsyncContextTest` and `HttpServiceAsyncContextTest` to make sure `AsyncContext` propagation for blocking/async aggregated services is also tested. 9. Use `ParameterizedTest` where possible. Results: 1. `AsyncContext` initialized inside server's lifecycle observer is visible through filter chain and services. 2. Construction of server-side filter chain is consistent and sequential. 3. Filters are applied for servers with optional TLS.
daschl
approved these changes
Nov 18, 2024
There was a problem hiding this comment.
Change looks good to me, I think it is an improvement also in follow-ability through the filter bootstrapping of the server. Just one nit, feel free to ignore.
One separate thing @idelpivnitskiy - since it changes the code quite a bit, just wanted to make sure that it is double checked that the new order is good and nothing ended up accidentally in the wrong spot of the filter chain. Doesn't look like it to me, just checking.
| return unmodifiableList(filters); | ||
| } | ||
|
|
||
| private List<StreamingHttpServiceFilterFactory> initNonOffloadsServiceFilters( |
There was a problem hiding this comment.
I would suggest renaming this method to something that includes the word default so its understood that those are the default ones always added
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
If users put any data into
AsyncContextinsideHttpLifecycleObserverconfigured viaHttpServerBuilder.lifecycleObserver(...)it won't be visible for filters and service becauseClearAsyncContextHttpServiceFilteris appended afterHttpLifecycleObserverServiceFilterinsideapplyInternalFilters.Modifications:
ClearAsyncContextHttpServiceFilterfrom the beginning ofnoOffloadServiceFilterstoapplyInternalFilters. However, this move discovered another bug in path that handlesnoOffloadServiceFilters.isEmpty()case because it never addsOffloadingFilter.listenForServicemethod to always make a copy of filters lists, then prepend/append internal filters to those copies in easy to read/understand way, and construct a single filtersStreamforbuildServicemethod.OffloadingFilterto act as a regular filter factory instead of taking all furtherserviceFiltersas pre-built factory.ClearAsyncContextHttpServiceFiltersingleton instance.OptionalSslNegotiatorbinder takes a raw service instead offilteredService(user-defined filters were never applied for this path).AbstractHttpServiceAsyncContextTestto testAsyncContextpropagation from lifecycle observer and non-offloading filters.AbstractHttpServiceAsyncContextTestto testAsyncContextinitialized by early/late connection acceptor is not visible at request path.BlockingHttpServiceAsyncContextTestandHttpServiceAsyncContextTestto make sureAsyncContextpropagation for blocking/async aggregated services is also tested.ParameterizedTestwhere possible.Results:
AsyncContextinitialized inside server's lifecycle observer is visible through filter chain and services.