-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Update NIO event loop creation to use Netty 4.2 API #3584 #3585
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
base: main
Are you sure you want to change the base?
Conversation
After upgrading to Netty 4.2 in #3405, the NIO event loop creation in DefaultEventLoopGroupProvider still used the old Netty 4.1 API (new NioEventLoopGroup()), which creates event loops incompatible with applications using Netty 4.2's new IO model. This commit introduces NioProvider following the same pattern as EpollProvider, KqueueProvider, and IOUringProvider. The provider creates NIO event loop groups using MultiThreadIoEventLoopGroup with NioIoHandler.newFactory(), ensuring compatibility with Netty 4.2. Fixes #3584
The test was expecting NioEventLoopGroup but now we return MultiThreadIoEventLoopGroup (Netty 4.2 API). Updated the test to use EventLoopGroup interface instead of the concrete type.
|
Fixed the test failure. The issue was that The fix is backward compatible - the test still requests |
…3584 Updated tests to use MultithreadEventLoopGroup instead of NioEventLoopGroup for variables that need to call executorCount(). The allocate() method now returns MultiThreadIoEventLoopGroup (Netty 4.2 API) instead of NioEventLoopGroup.
|
Fixed another test file ( |
|
✅ All CI checks are now passing! The PR is ready for review. Summary of changes:
This ensures Lettuce is fully compatible with Netty 4.2's new IO model and eliminates the |
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.
We need to stop using the deprecated NioEventLoopGroup.class everywhere in our code.
We might need to revisit Transports and change the defaults with the NioProvider util .
| factoryProvider.getThreadFactory("lettuce-eventExecutorLoop")); | ||
| } | ||
|
|
||
| if (NioEventLoopGroup.class.equals(type)) { |
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.
We still use deprecated class here.
…3584 - Updated DefaultEventLoopGroupProvider to use NioProvider.getResources().matches() instead of explicit NioEventLoopGroup.class check - Updated Transports.eventLoopGroupClass() to return NioProvider.getResources().eventLoopGroupClass() instead of NioEventLoopGroup.class - This removes all references to the deprecated NioEventLoopGroup.class from the main source code (except in NioProvider.matches() for backward compatibility)
|
✅ Addressed the code review feedback to stop using deprecated Changes made:
Result:
Ready for another review! |
Description
This PR fixes the incomplete Netty 4.2 migration for NIO event loop creation.
After upgrading to Netty 4.2 in #3405, the NIO event loop creation in
DefaultEventLoopGroupProviderstill used the old Netty 4.1 API (new NioEventLoopGroup()), which creates event loops incompatible with applications using Netty 4.2's new IO model.Changes
NioProvider: Following the same pattern asEpollProvider,KqueueProvider, andIOUringProviderDefaultEventLoopGroupProvider: Now usesNioProvider.getResources()to create NIO event loop groupsMultiThreadIoEventLoopGroupwithNioIoHandler.newFactory()Impact
This ensures that NIO event loop creation is consistent with the other transports and compatible with Netty 4.2's new IO model. Applications using Netty 4.2 event loops will no longer encounter
IllegalStateException: incompatible event loop typeerrors when using Lettuce.Testing
Fixes #3584