Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Return failed EventLoopFuture when getOption(...) / setOption(...) is… #198

Merged
merged 2 commits into from
Mar 20, 2018

Conversation

normanmaurer
Copy link
Member

… called on a closed Channel

Motivation:

We should return a failed EventLoopFuture when getOption(...) / setOption(...) is called on a closed Channel as otherwise it may not be safe to modify the Channel ioptions after its closed.

Modifications:

  • Add guards that check if the Channel is still open and if not fail the operation.
  • Add testcase

Result:

Calling getOption(...) / setOption(...) on a closed Channel fails.


try assertSetGetOptionOnOpenAndClosed(channel: clientChannel, option: ChannelOptions.allowRemoteHalfClosure, value: true)
try assertSetGetOptionOnOpenAndClosed(channel: serverChannel, option: ChannelOptions.backlog, value: 100)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you do this for datagram channels too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@normanmaurer normanmaurer force-pushed the options_closed_channel branch from 6c6fa7f to 4239216 Compare March 20, 2018 14:56
@normanmaurer
Copy link
Member Author

@Lukasa always forget about DatagramChannel. Added a test as well.

@normanmaurer normanmaurer force-pushed the options_closed_channel branch 2 times, most recently from ed36067 to 872f13f Compare March 20, 2018 14:57
Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good but not the right error

@@ -284,6 +284,10 @@ class BaseSocketChannel<T: BaseSocket>: SelectableChannel, ChannelCore {
fileprivate func setOption0<T: ChannelOption>(option: T, value: T.OptionType) throws {
assert(eventLoop.inEventLoop)

guard isOpen else {
throw ChannelError.alreadyClosed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.alreadyClosed is defined as only close on already closed channel. This should be ioOnClosedChannel

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

screen shot 2018-03-20 at 3 42 04 pm

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weissi fixed

… called on a closed Channel

Motivation:

We should return a failed EventLoopFuture when getOption(...) / setOption(...) is called on a closed Channel as otherwise it may not be safe to modify the Channel ioptions after its closed.

Modifications:

- Add guards that check if the Channel is still open and if not fail the operation.
- Add testcase

Result:

Calling getOption(...) / setOption(...) on a closed Channel fails.
@normanmaurer normanmaurer force-pushed the options_closed_channel branch from d803367 to 7e9824c Compare March 20, 2018 15:48
Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@Lukasa
Copy link
Contributor

Lukasa commented Mar 20, 2018

Rebase and merge

@normanmaurer
Copy link
Member Author

@swift-nio-bot test this please

@normanmaurer normanmaurer merged commit 17d5da5 into apple:master Mar 20, 2018
@normanmaurer normanmaurer deleted the options_closed_channel branch March 20, 2018 16:38
@normanmaurer normanmaurer added this to the 1.3.0 milestone Mar 20, 2018
@normanmaurer normanmaurer added the 🔨 semver/patch No public API change. label Mar 20, 2018
weissi pushed a commit to weissi/swift-nio that referenced this pull request Jun 13, 2020
Motivation:

Sometimes folks will want to print things about the HTTP2StreamChannel,
and we should give them the things we can safely get. We also may want a
shorter log for NIOHTTP2Handler as the standard description, with the
wall of text available in the debug description.

Modifications:

- Conform HTTP2StreamChannel to CustomStringConvertible
- Conform NIOHTTP2Handler to CustomDebugStringConvertible.

Result:

Even better debug info.
weissi pushed a commit to weissi/swift-nio that referenced this pull request Feb 3, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants