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

If connectSocket() completes directly we need to mark channel as active #205

Merged
merged 1 commit into from
Mar 21, 2018

Conversation

normanmaurer
Copy link
Member

Motivation:

If connectSocket() completes directly we failed to mark the Channel active by calling becomeActive0(...).

Modifications:

Correctly call becomeActive0(...)

Result:

Mark Channel active if connect succeed directly.

@normanmaurer normanmaurer requested review from weissi and Lukasa March 20, 2018 18:24
@normanmaurer
Copy link
Member Author

Need to add a test...

Motivation:

If connectSocket() completes directly we failed to mark the Channel active by calling becomeActive0(...).

Modifications:

Correctly call becomeActive0(...)

Result:

Mark Channel active if connect succeed directly.
@normanmaurer normanmaurer force-pushed the non_delayed_connect_active branch from 0a20349 to b71b55d Compare March 20, 2018 18:42
@normanmaurer
Copy link
Member Author

@weissi @Lukasa ok test added

@normanmaurer normanmaurer self-assigned this Mar 20, 2018
@normanmaurer normanmaurer added the 🔨 semver/patch No public API change. label Mar 20, 2018
@normanmaurer normanmaurer added this to the 1.3.0 milestone Mar 20, 2018
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.

So I’m tentatively ok with this, but frankly I think we should investigate whether this can just call finishConnectSocket so that there is only one code path for connect management.

@normanmaurer
Copy link
Member Author

@Lukasa we can investigate but I think we really should not do this as calling getsockopt here is really a waste if not needed and its an easy "fix"

@Lukasa
Copy link
Contributor

Lukasa commented Mar 21, 2018

Ok :)

@Lukasa Lukasa merged commit 8bf013d into apple:master Mar 21, 2018
@normanmaurer normanmaurer deleted the non_delayed_connect_active branch April 23, 2018 12:10
weissi pushed a commit to weissi/swift-nio that referenced this pull request Jun 13, 2020
Motivation:

As part of apple#202 we added support for configuring the target window size
of stream channels. That was plumbed through into outbound channels, but
we missed the inbound ones, meaning that this only really worked for
clients, not servers. That hardly seems fair!

Modifications:

Pass target window size to inbound channels too.

Result:

We can control target window size of inbound channels.
# 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.

2 participants