Conversation
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Pull request overview
This PR appears to address configuration handling and reloading for the Lambda invoker, with some cleanup of schema formatting and handler behavior.
Changes:
- Fixes formatting of the
lambda-invokerJSON schema file. - Refactors
LambdaInvokerConfigto cache the underlying mapped configuration, introduce a singleton-styleloadmethod, and register the module viaModuleRegistry. - Updates
LambdaFunctionHandlerto use instance fields for config/client, factor out client initialization, fix the API call attempt timeout units, and add dynamic config reload behavior insidehandleRequest.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
lambda-invoker/src/main/resources/config/lambda-invoker-schema.json |
Minor formatting change to the closing brace of the lambda-invoker JSON schema. |
lambda-invoker/src/main/java/com/networknt/aws/lambda/LambdaInvokerConfig.java |
Introduces a cached mappedConfig map and a singleton-style load(String) method with ModuleRegistry registration for the lambda-invoker config. |
lambda-invoker/src/main/java/com/networknt/aws/lambda/LambdaFunctionHandler.java |
Refactors handler to use instance config/client, centralizes client creation in initClient, corrects timeout units, adjusts logging, and adds dynamic reload of LambdaInvokerConfig and client inside handleRequest. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Override | ||
| public void handleRequest(HttpServerExchange exchange) throws Exception { | ||
| LambdaInvokerConfig newConfig = LambdaInvokerConfig.load(); | ||
| if(newConfig != config) { | ||
| synchronized (this) { | ||
| newConfig = LambdaInvokerConfig.load(); | ||
| if(newConfig != config) { | ||
| config = newConfig; | ||
| client = initClient(config); | ||
| if(config.isMetricsInjection()) { |
There was a problem hiding this comment.
When the configuration changes, handleRequest replaces the client instance by calling initClient(config) but never closes the previous LambdaAsyncClient, which manages its own resources and threads. To avoid leaking connections/threads across reloads, close the existing client (if non-null) before replacing it, or reuse a single client instance per handler and update only the configuration-dependent state that actually needs to change.
| @Override | ||
| public void handleRequest(HttpServerExchange exchange) throws Exception { | ||
| LambdaInvokerConfig newConfig = LambdaInvokerConfig.load(); | ||
| if(newConfig != config) { | ||
| synchronized (this) { | ||
| newConfig = LambdaInvokerConfig.load(); | ||
| if(newConfig != config) { | ||
| config = newConfig; | ||
| client = initClient(config); | ||
| if(config.isMetricsInjection()) { | ||
| // get the metrics handler from the handler chain for metrics registration. If we cannot get the | ||
| // metrics handler, then an error message will be logged. | ||
| Map<String, HttpHandler> handlers = Handler.getHandlers(); | ||
| metricsHandler = (AbstractMetricsHandler) handlers.get(MetricsConfig.CONFIG_NAME); | ||
| if(metricsHandler == null) { | ||
| logger.error("An instance of MetricsHandler is not configured in the handler.yml."); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The new dynamic reload path in handleRequest (reloading LambdaInvokerConfig and reinitializing the LambdaAsyncClient) is not covered by tests in LambdaFunctionHandlerTest, which currently only exercises request-event serialization. Given that this logic affects live request handling and resource management, consider adding a unit or integration test that exercises a config change and verifies that the handler uses the updated configuration and client without errors.
| public static LambdaInvokerConfig load(String configName) { | ||
| if (CONFIG_NAME.equals(configName)) { | ||
| Map<String, Object> mappedConfig = Config.getInstance().getJsonMapConfig(configName); | ||
| if (instance != null && instance.getMappedConfig() == mappedConfig) { | ||
| return instance; | ||
| } | ||
| synchronized (LambdaInvokerConfig.class) { | ||
| mappedConfig = Config.getInstance().getJsonMapConfig(configName); | ||
| if (instance != null && instance.getMappedConfig() == mappedConfig) { | ||
| return instance; | ||
| } | ||
| instance = new LambdaInvokerConfig(configName); | ||
| ModuleRegistry.registerModule(CONFIG_NAME, LambdaInvokerConfig.class.getName(), Config.getNoneDecryptedInstance().getJsonMapConfigNoCache(CONFIG_NAME), null); | ||
| return instance; | ||
| } |
There was a problem hiding this comment.
The lazy singleton implementation here publishes instance from within a synchronized block but reads it (and its non-final fields like region, functions, etc.) without any synchronization or volatile, which violates Java's safe-publication rules and can expose other threads to a partially-constructed LambdaInvokerConfig instance. To make this thread-safe, either declare instance as volatile (or use an AtomicReference) and keep the double-checked pattern, or drop the unsynchronized fast path and always access instance behind the same class-level lock.
| LambdaInvokerConfig config = LambdaInvokerConfig.load(); | ||
| this.client = initClient(config); | ||
| if(config.isMetricsInjection()) { |
There was a problem hiding this comment.
In the constructor, the local variable LambdaInvokerConfig config = LambdaInvokerConfig.load(); shadows the instance field config, so the field remains null until the first handleRequest call triggers the reload block. This is confusing and unnecessarily causes the client to be reinitialized on the first request; consider assigning the loaded config to the instance field (e.g., this.config = LambdaInvokerConfig.load();) and reusing it to avoid shadowing and redundant initialization.
| LambdaInvokerConfig config = LambdaInvokerConfig.load(); | |
| this.client = initClient(config); | |
| if(config.isMetricsInjection()) { | |
| this.config = LambdaInvokerConfig.load(); | |
| this.client = initClient(this.config); | |
| if (this.config.isMetricsInjection()) { |
1 participant
No description provided.