Take closeFuture into account when closing EventLoopGroup #231
Conversation
normanmaurer
force-pushed
the
closefuture_grace
branch
from
18eb7d4 to
cb28d82
Compare
Sources/NIO/Selector.swift
Outdated
| return chan.close() | ||
| return chan.close().then { | ||
| chan.closeFuture | ||
| } |
There was a problem hiding this comment.
This has the effect of firing the future early if the close errors. We probably don't want to do that: presumably we should just close() and then return chan.closeFuture, rather than chaining through then?
There was a problem hiding this comment.
@Lukasa yeah you are right... let me do this.
Tests/NIOTests/EventLoopTest.swift
Outdated
| XCTAssertFalse(channel.closeFuture.isFulfilled) | ||
| try group.syncShutdownGracefully() | ||
| XCTAssertTrue(channel.closeFuture.isFulfilled) | ||
| XCTAssertFalse(channel.isActive) |
There was a problem hiding this comment.
This test would probabilistically pass before the given change: it relies on a race between syncShutdownGracefully returning and the check on the state of channel.closeFuture and channel.isActive. These checks should probably be done by inserting a channel handler that catches close and checks whether syncShutdownGracefully has returned yet. It may even want to wait a few ms to be sure.
There was a problem hiding this comment.
@Lukasa good idea... I think we should add the check in channelInactive(...) tho. WDYT ?
There was a problem hiding this comment.
Actually handlerRemoved(...)
Motiviation: When the Selector is closed we also close the registered Channels and notify the future once all the close operations complete. The problem here is that while the close operation may be complete already there may still be events flowing through the ChannelPipeline. The only way to ensure this not happens anymore is to take the closeFuture into account of each Channel (as the closeFuture will only be notified once all is done for a Channel). Modification: - Also take the closeFuture into account per Channel - Add test Result: Safer shutdown of EventLoopGroup.
normanmaurer
force-pushed
the
closefuture_grace
branch
from
cb28d82 to
25cdd3d
Compare
Motiviation:
When the Selector is closed we also close the registered Channels and notify the future once all the close operations complete. The problem here is that while the close operation may be complete already there may still be events flowing through the ChannelPipeline. The only way to ensure this not happens anymore is to take the closeFuture into account of each Channel (as the closeFuture will only be notified once all is done for a Channel).
Modification:
Result:
Safer shutdown of EventLoopGroup.