Fix origins reload thread leak #290
Conversation
| @@ -68,7 +68,7 @@ private StyxHttpClient(NettyConnectionFactory connectionFactory, Builder paramet | |||
| /** | |||
| * Indicates that a request should be sent using secure {@code https} protocol. | |||
| * | |||
| * @return a {@HttpClient.Transaction} instance that allows fluent method chaining | |||
| * @return a {@link HttpClient.Transaction} instance that allows fluent method chaining | |||
There was a problem hiding this comment.
I had missed this one: I don't think it's common practice to include links in the 'returns' line: doesn't the line with the signature already contain a link to the returned object ? Maybe we can simplify the javadoc for this class.
There was a problem hiding this comment.
Looks like there are no changes related to the PR anyway, I'll remove them and we can resolve this another time.
| /** | ||
| * Check the health of an origin. | ||
| * | ||
| * @param client TODO |
| @@ -37,10 +38,11 @@ | |||
| * | |||
| * @param id the backend service id | |||
| * @param healthCheckConfig configuration of the health check | |||
| * @param supplier of the actual function which decided if the origin is healthy or not | |||
| * @param healthCheckFunction of the actual function which decided if the origin is healthy or not | |||
There was a problem hiding this comment.
Can we improve the description and remove the TODO?
| * @return the monitor of the origin | ||
| */ | ||
| OriginHealthStatusMonitor create(Id id, HealthCheckConfig healthCheckConfig, Supplier<OriginHealthCheckFunction> supplier); | ||
| OriginHealthStatusMonitor create(Id id, HealthCheckConfig healthCheckConfig, Supplier<OriginHealthCheckFunction> healthCheckFunction, HttpClient client); |
There was a problem hiding this comment.
Since we have moved the HttpClient creation outside of healthCheckFunction(), do we still need to use a supplier for the latter? I'm guessing it was used to lazily create the HttpClient.
There was a problem hiding this comment.
I tried removing the Supplier, but this caused tests to fail. I put it back since the change was out of the scope of this task.
| @@ -154,20 +145,49 @@ public void onChange(Registry.Changes<BackendService> changes) { | |||
| .hostClientFactory(hostClientFactory) | |||
| .build(); | |||
|
|
|||
| pipeline = new ProxyToClientPipeline(newClientHandler(backendService, inventory, originStatsFactory), inventory); | |||
| pipeline = new ProxyToClientPipeline(newClientHandler(backendService, inventory, originStatsFactory), () -> { | |||
| inventory.close(); | |||
There was a problem hiding this comment.
It seems here (and above in return runAsync(() -> {... ) we might be not following the principle that says we should keep statements in a function at the same level of abstraction. Following up on that, it also seems the pipeline could contain the logic to close itself instead of transferring the responsibility to the caller.
There was a problem hiding this comment.
The idea was to avoid bloating the ProxyToClientPipeline constructor signature by providing a kind of listener that can react to close events, but I am open to alternatives.
| try { | ||
| hostHealthMonitorExecutor.awaitTermination(10, SECONDS); | ||
| } catch (InterruptedException e) { | ||
| throw propagate(e); |
There was a problem hiding this comment.
Should we set the thread interrupted flag before propagating?
Also IIRC Throwables.propagate is deprecated from recent Guava versions.
There was a problem hiding this comment.
It seems that the ForkJoinPool clears the thread interruption status (in awaitWork) so it's not a huge issue if we don't clear the status, but the path execution is different and we should never discard the interruption status as a principle.
However, maybe we should not use the JoinFork pool for blocking tasks?
|
|
||
| private StyxHttpClient healthCheckClient(BackendService backendService) { | ||
| ConnectionSettings connectionSettings = new ConnectionSettings( | ||
| backendService.connectionPoolConfig().connectTimeoutMillis()); |
There was a problem hiding this comment.
ConnectionSettings class in created unnecessarily. The StyxHttpClient.connectTimeout takes an `int.
| * <p> | ||
| * Two {@link ItemsDump} objects can be diffed to quickly see the difference between two collections. | ||
| */ | ||
| public class ItemsDump { |
There was a problem hiding this comment.
I think this file should go into styx-e2e-testsupport.
| /** | ||
| * Helper methods for looking at threads. | ||
| */ | ||
| public final class Threads { |
There was a problem hiding this comment.
This too probably belongs to e2e-testsupport module.
| } | ||
|
|
||
| describe("Reload Origins Endpoint") { | ||
| it("should not leak threads") { |
There was a problem hiding this comment.
- Be careful with wording of the test case name:
describe("Styx health checking") ...
it ("cleans up resources after origins reload")
- Also please raise a GitHub issue for this problem, and add the issue number in a comment to this test case. The GitHub issue will become handy if this ever needs to be double committed to 0.7 branch in future.
There was a problem hiding this comment.
Issue created: #291
We have a bug in which thread pools are created each time origins are reloaded and never shut down.
This PR fixes the bug and adds a couple of small utility classes in the
e2e-suitethat may help with debugging & testing in the future.