Skip to content

don't just drop Channels on the floor if registration fails #413

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 22, 2018
Merged

don't just drop Channels on the floor if registration fails #413

merged 1 commit into from
May 22, 2018

Conversation

@weissi
Copy link
Member

@weissi weissi commented May 17, 2018

Motivation:

If registration of a Channel fails we need to close that Channel.
Previously we'd just drop it on the floor which triggers an assertion
that an open channel got leaked.

Modifications:

In cases where the Channel registration fails, we need to close that
channel.

Result:

No crashes if registration fails.

@weissi weissi requested review from Lukasa and normanmaurer and removed request for Lukasa May 17, 2018 18:14
normanmaurer
normanmaurer requested changes May 17, 2018
View reviewed changes
Tests/NIOTests/ChannelTests.swift Outdated
typealias OutboundIn = Never

func register(ctx: ChannelHandlerContext, promise: EventLoopPromise<Void>?) {
promise?.fail(error: RegistrationFailedError.error)
Copy link
Member

@normanmaurer normanmaurer May 17, 2018

Choose a reason for hiding this comment

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

@weissi maybe use ! as it should never be nil.

Tests/NIOTests/ChannelTests.swift
}
.bind(host: "localhost", port: 0).wait()
XCTFail("shouldn't be reached")
XCTAssertNoThrow(try serverChannel.close().wait())
Copy link
Member

@normanmaurer normanmaurer May 17, 2018

Choose a reason for hiding this comment

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

Remove this one as it should never be reached anyway ?

Copy link
Member Author

@weissi weissi May 18, 2018

Choose a reason for hiding this comment

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

I added it because if it fails, not closing it leads to a crash (assert) and then hides the actual bug. I’d leave it in but am also happy to take out.
You know, xctest doesn’t stop on failed assert...

Copy link
Member

@normanmaurer normanmaurer May 18, 2018

Choose a reason for hiding this comment

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

ah right... Ignore me then (or maybe add a comment) :)

@tomerd
Copy link
Member

tomerd commented May 17, 2018

@swift-nio-bot test this please

@vlm
Copy link
Contributor

vlm commented May 17, 2018

19:16:20 Test Case 'ChannelTests.testFailedRegistrationOfAcceptedSocket' started at 2018-05-17 19:16:20.374
19:16:20 /code/Tests/NIOTests/ChannelTests.swift:2405: error: ChannelTests.testFailedRegistrationOfAcceptedSocket : XCTAssertNoThrow failed: threw error "alreadyClosed" -

Lukasa
Lukasa requested changes May 17, 2018
View reviewed changes
Tests/NIOTests/ChannelTests.swift Outdated
.connect(to: serverChannel.localAddress!)
.wait()
defer {
XCTAssertNoThrow(try clientChannel.close().wait())
Copy link
Contributor

@Lukasa Lukasa May 17, 2018

Choose a reason for hiding this comment

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

There's a race in this close, racing with the close in the accepted server socket. Better to just wait for the closeFuture on this channel, which has the nice side effect of confirming that the channel actually did die.

Sources/NIO/Bootstrap.swift
group: childEventLoopGroup,
protocolFamily: address.protocolFamily)
}
return bind0(makeServerChannel: makeChannel) { (eventGroup, serverChannel) in
Copy link
Contributor

@Lukasa Lukasa May 17, 2018

Choose a reason for hiding this comment

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

Should bind0 do this error handling? There are a few other paths through this bootstrap that didn't get changed here but that still do registrations.

Copy link
Member Author

@weissi weissi May 21, 2018

Choose a reason for hiding this comment

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

done now, thanks

@Lukasa Lukasa added the 🔨 semver/patch No public API change. label May 17, 2018
@weissi weissi force-pushed the jw-dont-drop-reg-fails branch from 0fbcdc4 to f52da00 Compare May 18, 2018 16:19
@Lukasa Lukasa mentioned this pull request May 19, 2018
Closed
@weissi weissi force-pushed the jw-dont-drop-reg-fails branch from f52da00 to 71cdf87 Compare May 21, 2018 18:03
@weissi
Copy link
Member Author

weissi commented May 21, 2018

@swift-nio-bot test this please

@weissi
Copy link
Member Author

weissi commented May 21, 2018

@normanmaurer / @Lukasa updated this PR

@Lukasa
Copy link
Contributor

Lukasa commented May 21, 2018

This has regressed allocations on 4.1, it seems.

@weissi
Copy link
Member Author

weissi commented May 21, 2018 •
edited
Loading

@Lukasa sadly that is expected as I added extra future related allocations. But should only be per connection not per request. Will change the docket setup in this PR tomorrow :(

Wow, looks like 16 allocations more per connection pair. That’s a lot. Will look into that tomo.

Btw, we only check allocations on 4.1 (and not 4.0) hence only those are failing

@weissi
don't just drop Channels on the floor if registration fails
f3e038c 
Motivation:

If registration of a Channel fails we need to close that Channel.
Previously we'd just drop it on the floor which triggers an assertion
that an open channel got leaked.

Modifications:

In cases where the Channel registration fails, we need to close that
channel.

Result:

- No crashes if registration fails.
- fixes apple#417
@weissi weissi force-pushed the jw-dont-drop-reg-fails branch from 71cdf87 to f3e038c Compare May 22, 2018 09:27
Lukasa
Lukasa approved these changes May 22, 2018
View reviewed changes
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

LGTM, ship it.

@Lukasa Lukasa added this to the 1.7.1 milestone May 22, 2018
@weissi weissi merged commit ec880b0 into apple:master May 22, 2018
@weissi weissi deleted the jw-dont-drop-reg-fails branch May 22, 2018 10:29
weissi added a commit to weissi/swift-nio that referenced this pull request May 22, 2018
@weissi
in the registration failed tests, also delay close
1ed9960 
Motivation:

With apple#413 and apple#415 merged, we can turn up the level of nastiness in the
ChannelTests and delay the close that happens if registration fails in a
Bootstrap.

Modifications:

Delay close in the three registration fails ChannelTests

Result:

Tests are tougher.
@weissi weissi mentioned this pull request May 22, 2018
Merged
Lukasa pushed a commit that referenced this pull request May 22, 2018
@weissi @Lukasa
in the registration failed tests, also delay close (#425)
e94315d 
Motivation:

With #413 and #415 merged, we can turn up the level of nastiness in the
ChannelTests and delay the close that happens if registration fails in a
Bootstrap.

Modifications:

Delay close in the three registration fails ChannelTests

Result:

Tests are tougher.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@normanmaurer normanmaurer normanmaurer approved these changes

@Lukasa Lukasa Lukasa approved these changes

Assignees

No one assigned

Labels

🔨 semver/patch No public API change.

Projects

None yet

Milestone

1.7.1

Development

Successfully merging this pull request may close these issues.

5 participants

@weissi @tomerd @vlm @Lukasa @normanmaurer