-
-
Notifications
You must be signed in to change notification settings - Fork 15.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace EmbeddedChannel
's 'frozen time' feature with Ticker
#13169
Conversation
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.
common/src/main/java/io/netty5/util/concurrent/SystemTicker.java
Outdated
Show resolved
Hide resolved
lgtm, but this change only applies to EmbeddedEventLoop still, do you plan to move the ticker to "normal" event loops later? |
Thanks for taking a look, @yawkat 🙇 Yeah, that's the plan. |
i wish fixing bad sleep was always this easy |
common/src/main/java/io/netty5/util/concurrent/SingleThreadEventExecutor.java
Outdated
Show resolved
Hide resolved
…ntExecutor.java Co-authored-by: jchrys <jchrys@me.com>
@normanmaurer @chrisvest @hyperxpro Thoughts on this change? Let me continue on this path if it looks good to you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API is a lot better than before! ^^
void newMockTickerShouldReturnDefaultMockTicker() { | ||
assertTrue(Ticker.newMockTicker() instanceof DefaultMockTicker); | ||
} | ||
@Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Test | |
@Test |
common/src/test/java/io/netty5/util/concurrent/DefaultMockTickerTest.java
Outdated
Show resolved
Hide resolved
common/src/test/java/io/netty5/util/concurrent/DefaultMockTickerTest.java
Outdated
Show resolved
Hide resolved
@hyperxpro Thanks for reviewing. Let me address the comments. |
@hyperxpro Resolved all comments. PTAL again. 🙇 |
Will merge this if I don't get any comments until I check this PR again, like in 3 days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for delay ;/
LGTM!
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:
Ticker
andMockTicker
and their default implementations.EmbeddedChannel
andEmbeddedEventLoop
to useTicker
instead of its own time manipulation API.freezeTime()
andunfreezeTime()
because it can easily be replicated withadvance()
.Result:
Ticker
implementation when constructing anEmbeddedChannel
, which is more flexible than the previous API.EmbeddedChannel
has been removed in favor of the new API.AbstractScheduledEventExecutor.getCurrentTimeNanos
#12827. However, this PR didn't updateIdleStateHandler
or any other time-sensitive logic in Netty, which is left as future work.