Expose AbstractScheduledEventExecutor.getCurrentTimeNanos
#12827
Comments
|
Project reactor has a similar idea in its Scheduler class: https://projectreactor.io/docs/core/release/api/reactor/core/scheduler/Scheduler.html#now-java.util.concurrent.TimeUnit- |
|
I need to think a bit more about it |
yawkat
added a commit
to micronaut-projects/micronaut-core
that referenced
this issue
Oct 19, 2022
This PR adds a new connection pooling infrastructure to the HTTP client. The biggest changes are in ConnectionManager, which contains network setup code, and in the new PoolResizer class, which handles pool dynamics (it calls into ConnectionManager.Pool to assign requests, establish new connections, etc, but does no network setup by itself). Changes in no particular order: - The `http-version` config setting has been replaced by two settings. `plaintext-mode` applies to `http` URLs and can be set to either `HTTP_1` or `H2C`. `alpn-modes` applies to `https` URLs, and determines the protocols that should be supported in protocol negotiation (it's a list, with the supported values `h2` and `http/1.1`). The old http-version setting remains for compatibility, and should remain for 4.0. It is also used in some test cases. - connection pooling now applies to all connections (except websockets), not just exchange calls, and it is enabled by default. - HTTP2 connections now use a channel-per-stream model (like #6842), and allow concurrent requests on one connection. - Connection pool configuration is more fine-grained, with separate settings for HTTP1 and HTTP2. - Pooled connections are now tracked using netty's resource leak detection, to avoid "dangling" connections in the pool (note: resource leak detection is still only tested widely for the tests in the http-server-netty module atm). - One minor fix in the http server that came up in the test suite because connection pooling is on by default now. There are still some TODOs in this PR. Some relate to pending netty improvements (netty/netty#12827 , netty/netty#12830), but work fine for the moment. Some are future improvements that could be made, but that I want to do in separate PRs.
|
How about introducing something similiar to |
|
It's actually something I wanted to implement in our codebase to reduce the overall test run time. Let me try to come up with one soon. |
Correct. I want to make it generic enough so it can be used in more places than just |
trustin
added a commit
that referenced
this issue
Jan 30, 2023
Motivation:
Netty currently lacks a standard abstraction for time source, as known
as 'ticker' or 'clock'. As a result, Netty relies on ad-hoc overridable
protected methods for 'time getter' methods, which gives hard time to
users who want to test the time-sensitive logic that involes Netty.
Modifications:
- Added `Ticker` and `MockTicker` and their default implementations.
- (Breaking) Updated `EmbeddedChannel` and `EmbeddedEventLoop` to use
`Ticker` instead of its own time manipulation API.
- Removed the old time manipulation API.
- Note that I removed `freezeTime()` and `unfreezeTime()` because it
can easily be replicated with `advance()`.
Result:
- Netty now has a better abstraction for scheduling and testing
time-sensitive logic.
- A user can now specify system, mock or custom `Ticker` implementation
when constructing an `EmbeddedChannel`, which is more flexible than
the previous API.
- (Breaking) The previous time-manipulation API in `EmbeddedChannel`
has been removed in favor of the new API.
- Partially resolves #12827. However, this PR didn't update
`IdleStateHandler` or any other time-sensitive logic in Netty, which
is left as future work.
trustin
added a commit
that referenced
this issue
Mar 18, 2023
Motivation: Netty currently lacks a standard abstraction for time source, as known as 'ticker' or 'clock'. As a result, Netty relies on ad-hoc overridable protected methods for 'time getter' methods, which gives hard time to users who want to test the time-sensitive logic that involes Netty. Modifications: - Added `Ticker` and `MockTicker` and their default implementations. - (Breaking) Updated `EmbeddedChannel` and `EmbeddedEventLoop` to use `Ticker` instead of its own time manipulation API. - Removed the old time manipulation API. - Note that I removed `freezeTime()` and `unfreezeTime()` because it can easily be replicated with `advance()`. Result: - Netty now has a better abstraction for scheduling and testing time-sensitive logic. - A user can now specify system, mock or custom `Ticker` implementation when constructing an `EmbeddedChannel`, which is more flexible than the previous API. - (Breaking) The previous time-manipulation API in `EmbeddedChannel` has been removed in favor of the new API. - Partially resolves #12827. However, this PR didn't update `IdleStateHandler` or any other time-sensitive logic in Netty, which is left as future work. Co-authored-by: jchrys <jchrys@me.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
It would be useful to expose the
getCurrentTimeNanosmethod to channel handlers, so that they can use it instead of callingSystem.nanoTimedirectly. The main purpose would be to make handlers work predictably with the EmbeddedEventLoop time manipulation methods (advance, freeze).A good example is IdleStateHandler. It uses both event loop scheduling, and then nanoTime to confirm the timeout. If the event loop was advanced with
advanceBy, the scheduled task will run, but the nanoTime will not show a timeout. For the unit test of IdleStateHandler, this nanoTime call is already overridden (it's in a package-private method), but when writing an external test that involves IdleStateHandler, this is not available. By exposinggetCurrentTimeNanos, we would have a "single source of truth" when it comes to time, circumventing this issue.Another minor advantage would be that handlers can take advantage of the nanoTime offset that the event loops implement, but this is not worth a change on its own.
The right place for this api would probably be
EventExecutor(maybeEventExecutorGroup? does it make sense to have different times for each event loop in a group?). What do you think, is this worth adding?The text was updated successfully, but these errors were encountered: