feat: allow replacement of the default serinus http adapter#227
feat: allow replacement of the default serinus http adapter#227francescovallone wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughIntroduces keyed adapter management and runtime replacement: AdapterContainer gains keyed add/replace APIs; ApplicationConfig tracks and can replace its server adapter and marks attachment; Application and factory functions accept optional adapters and propagate adapter configuration; tests use a tracking adapter to verify lifecycle and replacement. Changes
Sequence DiagramsequenceDiagram
actor User
participant Factory
participant AppConfig as ApplicationConfig
participant AdapterContainer
participant SerinusApp as SerinusApplication
participant HttpAdapter
User->>Factory: createApplication(adapter: OptionalHttpAdapter)
activate Factory
alt adapter provided
Factory->>HttpAdapter: use provided adapter
else
Factory->>HttpAdapter: construct SerinusHttpAdapter
end
Factory->>AppConfig: new ApplicationConfig(serverAdapter)
activate AppConfig
AppConfig->>AdapterContainer: addAs(primaryHttpAdapterKey, serverAdapter)
activate AdapterContainer
AdapterContainer->>AdapterContainer: store adapter under key
deactivate AdapterContainer
AppConfig->>AppConfig: attachToApplication()
deactivate AppConfig
Factory->>SerinusApp: new SerinusApplication(config, adapter?)
activate SerinusApp
SerinusApp->>HttpAdapter: init(config)
activate HttpAdapter
HttpAdapter-->>SerinusApp: initialized
deactivate HttpAdapter
deactivate SerinusApp
deactivate Factory
Factory-->>User: SerinusApplication ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/serinus/lib/src/containers/adapters_container.dart`:
- Around line 32-35: The replace(String key, Adapter adapter) method currently
overwrites or inserts unconditionally into the _adapters map; change it to
ensure it only replaces existing entries: check _adapters.containsKey(key) and
if the key does not exist throw a clear error (e.g., StateError or
ArgumentError) instead of inserting, otherwise assign the adapter to
_adapters[key]; this prevents silent creation of a second entry when callers
meant to replace an existing adapter.
In `@packages/serinus/lib/src/core/application_config.dart`:
- Around line 120-123: The public serverAdapter setter allows callers to swap
adapters after Application construction which desynchronizes state; change the
setter on ApplicationConfig (serverAdapter) to prevent runtime swaps by either
making it private or by guarding it: detect if _container.applicationRef or the
cached RoutesResolver/lifecycle is already initialized and throw an error (or
no-op) instead of updating _serverAdapter and adapters.replace; ensure
references to _container.applicationRef, RoutesResolver, and the adapter
lifecycle are consulted so callers cannot change adapters via
app.config.serverAdapter after Application is constructed.
In `@packages/serinus/lib/src/core/application.dart`:
- Around line 73-80: The public isInitialized getter and the guard in
replaceHttpAdapter rely on a base _isInitialized flag, but
MicroserviceApplication.initialize() uses a separate _isInizialized field and
Application.initialize() sets _isInitialized too early; unify to a single
private flag (e.g. _isInitialized) used everywhere, remove the duplicate
_isInizialized, and ensure Application.initialize() only sets _isInitialized
after _container.init() (and any other startup steps) complete successfully so a
failed startup doesn't flip the flag; update
MicroserviceApplication.initialize() and any checks (including
replaceHttpAdapter(HttpAdapter)) to reference the unified _isInitialized.
- Around line 94-96: After swapping the primary adapter (setting
config.serverAdapter and _container.applicationRef and calling await
adapter.init(config)), rebuild the RoutesResolver so RouteResponseController
captures the new applicationRef; specifically, after the adapter init call
reinstantiate or refresh _routesResolver (the RoutesResolver instance that
constructs RouteResponseController) so reply/render/redirect use the new adapter
implementation instead of the old one.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62da0342-81df-4a2a-be51-31267008f295
📒 Files selected for processing (6)
packages/serinus/lib/src/containers/adapters_container.dartpackages/serinus/lib/src/containers/serinus_container.dartpackages/serinus/lib/src/core/application.dartpackages/serinus/lib/src/core/application_config.dartpackages/serinus/lib/src/core/factory.dartpackages/serinus/test/core/application_test.dart
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/serinus/lib/src/core/application.dart (2)
73-75:⚠️ Potential issue | 🟠 Major
isInitializedreturns incorrect value forMicroserviceApplication.The getter reads
_isInitializedfrom the base class, butMicroserviceApplication.initialize()(Lines 247-263) sets a separate_isInizializedfield (Line 211). This meansisInitializedwill returnfalsefor an initializedMicroserviceApplication.Consider removing
_isInizializedfromMicroserviceApplicationand having it callsuper.initialize()or directly set the base class field.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/serinus/lib/src/core/application.dart` around lines 73 - 75, The MicroserviceApplication has its own _isInizialized field that gets set in MicroserviceApplication.initialize(), while the base class exposes isInitialized that reads _isInitialized; remove the duplicate _isInizialized field from MicroserviceApplication and either call super.initialize() from MicroserviceApplication.initialize() or explicitly set the base class field (_isInitialized) in that method so that the inherited isInitialized getter reflects the correct initialized state (refer to MicroserviceApplication.initialize(), isInitialized, _isInitialized, and _isInizialized).
97-99:⚠️ Potential issue | 🔴 Critical
_routesResolveris not rebuilt after swapping the adapter.After updating
config.serverAdapterand_container.applicationRef(Lines 97-98), the existing_routesResolverstill holds a reference to the old adapter captured during its construction. This meansreply,render, andredirectinRouteResponseControllerwill continue using the old adapter.,
♻️ Proposed fix to rebuild the resolver
config.serverAdapter = adapter; _container.applicationRef = adapter; + _routesResolver = RoutesResolver(_container); await adapter.init(config);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/serinus/lib/src/core/application.dart` around lines 97 - 99, After swapping the adapter (updating config.serverAdapter and _container.applicationRef) and calling await adapter.init(config), rebuild the routes resolver so RouteResponseController's reply/render/redirect use the new adapter; locate where _routesResolver is constructed (the _routesResolver symbol) and reinstantiate it or call the existing factory/init method that creates it (so it captures the updated _container.applicationRef), ensuring this occurs immediately after adapter.init completes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/serinus/lib/src/core/application.dart`:
- Around line 89-99: The code closes currentAdapter (currentAdapter.isOpen ->
currentAdapter.close) before calling adapter.init(config), which can leave the
app without a working adapter if init throws; change the flow to initialize the
new adapter first (call adapter.init(config)) and only after successful init
replace config.serverAdapter and _container.applicationRef and then close the
old adapter (currentAdapter.close), or if you prefer to keep current order wrap
adapter.init(config) in a try/catch that on failure attempts to restore the old
adapter/state and logs an unrecoverable error; reference currentAdapter,
adapter.init(config), currentAdapter.close, config.serverAdapter and
_container.applicationRef when making the change.
- Around line 94-99: The code only preserves adapter.viewEngine when replacing
the HttpAdapter, which discards other mutable adapter state (notFoundHandler,
rawBody) defined on HttpAdapter; update the replacement logic around
adapter/viewEngine/currentViewEngine so you also copy adapter.notFoundHandler
and adapter.rawBody (and any other mutable fields on HttpAdapter) from the
previous adapter instance to the new adapter before assigning
config.serverAdapter/_container.applicationRef and calling adapter.init(config)
so those custom behaviors are preserved.
---
Duplicate comments:
In `@packages/serinus/lib/src/core/application.dart`:
- Around line 73-75: The MicroserviceApplication has its own _isInizialized
field that gets set in MicroserviceApplication.initialize(), while the base
class exposes isInitialized that reads _isInitialized; remove the duplicate
_isInizialized field from MicroserviceApplication and either call
super.initialize() from MicroserviceApplication.initialize() or explicitly set
the base class field (_isInitialized) in that method so that the inherited
isInitialized getter reflects the correct initialized state (refer to
MicroserviceApplication.initialize(), isInitialized, _isInitialized, and
_isInizialized).
- Around line 97-99: After swapping the adapter (updating config.serverAdapter
and _container.applicationRef) and calling await adapter.init(config), rebuild
the routes resolver so RouteResponseController's reply/render/redirect use the
new adapter; locate where _routesResolver is constructed (the _routesResolver
symbol) and reinstantiate it or call the existing factory/init method that
creates it (so it captures the updated _container.applicationRef), ensuring this
occurs immediately after adapter.init completes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a05a7d1a-df4e-4e74-b438-fe53d711ddad
📒 Files selected for processing (2)
packages/serinus/lib/src/containers/adapters_container.dartpackages/serinus/lib/src/core/application.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/serinus/lib/src/containers/adapters_container.dart
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/serinus/lib/src/core/application_config.dart (1)
138-143: Consider documenting adapter cleanup responsibility.The
replaceServerAdapter()method updates the backing field and the adapter container but does not close the previous adapter. Per context snippet 3 (adapters_container.dart:33-39),replace()simply overwrites the map entry.If the old adapter has been initialized, this could leak resources. Consider either:
- Closing the old adapter before replacement, or
- Documenting that callers are responsible for cleanup.
Given this is an
@internalmethod used during pre-initialization configuration (beforeattachToApplication()), the adapter is unlikely to be open yet. If that's the intentional design, a brief doc comment clarifying this would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/serinus/lib/src/core/application_config.dart` around lines 138 - 143, replaceServerAdapter currently overwrites the stored adapter without closing any previously set instance, which may leak resources; update replaceServerAdapter (and references to _serverAdapter and adapters.replace/AdapterContainer.primaryHttpAdapterKey) to either: 1) capture the existing _serverAdapter, call its close/shutdown/dispose method if non-null before assigning the new value and replacing the container entry, or 2) add a clear doc comment above replaceServerAdapter stating that this method is only used during pre-initialization (before attachToApplication) and therefore callers are responsible for adapter lifecycle/cleanup; pick one approach and apply consistently.packages/serinus/lib/src/core/application.dart (1)
233-242: Verify adapter close order inMicroserviceApplication.close().The close method iterates over
config.adapters.valuesand thenconfig.microservices. Since the primary HTTP adapter is registered in the adapters container viaaddAs(primaryHttpAdapterKey, ...), it will be closed during theconfig.adapters.valuesloop.This is correct behavior, but if any microservice depends on the HTTP adapter still being open for cleanup coordination, there could be ordering issues. Consider documenting the shutdown order or ensuring microservices don't have such dependencies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/serinus/lib/src/core/application.dart` around lines 233 - 242, MicroserviceApplication.close() currently closes config.adapters.values before config.microservices which will close the primary HTTP adapter (registered via addAs(primaryHttpAdapterKey, ...)) prior to microservice cleanup; to fix, either reorder shutdown so you iterate and await config.microservices close() before closing config.adapters.values (keeping shutdown() call after both), or add an explicit comment/documentation near the close() implementation that states the shutdown order and warns microservices not to rely on adapters being available during their close(); update the method body or add the inline doc next to MicroserviceApplication.close(), config.adapters.values, config.microservices, and addAs(primaryHttpAdapterKey) accordingly.packages/serinus/test/core/application_test.dart (1)
16-23: Add a test case that exercisesthrowOnListento verify error handling.The
throwOnListenparameter is implemented (line 48-50) but never set totruein any test, as evidenced by the// ignore: unused_element_parametercomment. Add a test that passesthrowOnListen: trueto verify the application handlesSocketExceptionduringlisten()gracefully, exercising the error handling path inSerinusApplication.serve()(lines 316-318).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/serinus/test/core/application_test.dart` around lines 16 - 23, Add a new test in application_test.dart that constructs the test adapter with throwOnListen: true (using the _TrackingAdapter constructor) and calls SerinusApplication.serve(), asserting that a SocketException thrown during listen() is handled gracefully (the serve call does not rethrow and the application transitions to the expected non-started/error state); this exercises the throwOnListen path and the SerinusApplication.serve() error handling for SocketException.packages/serinus/lib/src/containers/serinus_container.dart (1)
36-37: Consider makingapplicationReffinal or document the reconstruction requirement.Making
applicationRefmutable creates a theoretical desync risk if the entire adapter reference is replaced afterRoutesResolveris created. However, the current codebase shows only property-level mutations (e.g.,viewEnginesetter), which safely propagate because all holders share the same reference. Additionally, theApplication.containersetter already reconstructsRoutesResolverwhen needed.If runtime adapter replacement is not required, mark
applicationRefasfinalfor clarity. Otherwise, document that replacing the adapter requires reconstructing the container via the setter, or ensure all access paths use the current reference (rather than caching it).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/serinus/lib/src/containers/serinus_container.dart` around lines 36 - 37, The field applicationRef on SerinusContainer should either be made final or have clear documentation and safeguards about replacing the adapter: if adapter replacement is not needed, change the declaration of applicationRef to be final to prevent desyncs with RoutesResolver; otherwise, update docs/comments near applicationRef and/or modify code paths that cache the adapter (e.g., anywhere that constructs RoutesResolver using applicationRef) so they re-read the current Application.container (see Application.container setter) or trigger reconstruction of RoutesResolver when the adapter is swapped. Ensure the fix references applicationRef, RoutesResolver, and the Application.container setter so maintainers can find and apply the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/serinus/lib/src/containers/serinus_container.dart`:
- Around line 36-37: The field applicationRef on SerinusContainer should either
be made final or have clear documentation and safeguards about replacing the
adapter: if adapter replacement is not needed, change the declaration of
applicationRef to be final to prevent desyncs with RoutesResolver; otherwise,
update docs/comments near applicationRef and/or modify code paths that cache the
adapter (e.g., anywhere that constructs RoutesResolver using applicationRef) so
they re-read the current Application.container (see Application.container
setter) or trigger reconstruction of RoutesResolver when the adapter is swapped.
Ensure the fix references applicationRef, RoutesResolver, and the
Application.container setter so maintainers can find and apply the change.
In `@packages/serinus/lib/src/core/application_config.dart`:
- Around line 138-143: replaceServerAdapter currently overwrites the stored
adapter without closing any previously set instance, which may leak resources;
update replaceServerAdapter (and references to _serverAdapter and
adapters.replace/AdapterContainer.primaryHttpAdapterKey) to either: 1) capture
the existing _serverAdapter, call its close/shutdown/dispose method if non-null
before assigning the new value and replacing the container entry, or 2) add a
clear doc comment above replaceServerAdapter stating that this method is only
used during pre-initialization (before attachToApplication) and therefore
callers are responsible for adapter lifecycle/cleanup; pick one approach and
apply consistently.
In `@packages/serinus/lib/src/core/application.dart`:
- Around line 233-242: MicroserviceApplication.close() currently closes
config.adapters.values before config.microservices which will close the primary
HTTP adapter (registered via addAs(primaryHttpAdapterKey, ...)) prior to
microservice cleanup; to fix, either reorder shutdown so you iterate and await
config.microservices close() before closing config.adapters.values (keeping
shutdown() call after both), or add an explicit comment/documentation near the
close() implementation that states the shutdown order and warns microservices
not to rely on adapters being available during their close(); update the method
body or add the inline doc next to MicroserviceApplication.close(),
config.adapters.values, config.microservices, and addAs(primaryHttpAdapterKey)
accordingly.
In `@packages/serinus/test/core/application_test.dart`:
- Around line 16-23: Add a new test in application_test.dart that constructs the
test adapter with throwOnListen: true (using the _TrackingAdapter constructor)
and calls SerinusApplication.serve(), asserting that a SocketException thrown
during listen() is handled gracefully (the serve call does not rethrow and the
application transitions to the expected non-started/error state); this exercises
the throwOnListen path and the SerinusApplication.serve() error handling for
SocketException.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c48829b8-6df0-4f66-8b05-5c7d6c2d49a6
📒 Files selected for processing (6)
packages/serinus/lib/src/containers/adapters_container.dartpackages/serinus/lib/src/containers/serinus_container.dartpackages/serinus/lib/src/core/application.dartpackages/serinus/lib/src/core/application_config.dartpackages/serinus/lib/src/core/minimal/minimal_application.dartpackages/serinus/test/core/application_test.dart
|
I've been benchmarking Serinus today and found something important — dart:io has a hard performance ceiling at exactly 98,303 req/sec, regardless of connections: • 2000 conns peak: 98,303 req/sec The bottleneck is provably dart:io's HTTP parser, not Serinus itself. Serinus's architecture is already perfect — AOT + isolates scales beautifully to 72k avg req/sec at 10k connections, beating Bun (34,276 req/sec) on a laptop. |
2 participants
Description
Allow the user to change adapter to their application easily.
Type of change
Checklist:
Summary by CodeRabbit
New Features
Tests