Skip to content
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

Fire the pipelines error caught method when NWConnection's state changes to failed #187

Merged
merged 7 commits into from
Oct 25, 2023

Conversation

Cartisim
Copy link
Contributor

Fire the pipelines error caught method when NWConnection's state changes to failed

There seems to need to be a method fired when NSConnection's state changes to failed in order to notify other handlers in the pipeline.

Motivation:

When NWConnection fails the connection due to a number of reasons, one being that the clients devices loses internet and we want to receive the failed error in order to handle the applications behavior when the internet is lost. We need to fire the errorCaught method in order to notify handler's in the pipeline.

Modifications:

On line 277 of the file StateManagedNWConnectionChannel.swift I added the line of code to forward the error, namely...
self.pipeline.fireErrorCaught(error)

Result:

Our Handlers can now receive the error and handle the socket error state accordingly.

Copy link
Collaborator

@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.

Thanks for this patch! Can you add a unit test that validates that the error is appropriately caught? It's probably sufficient to connect to a port on localhost that definitely won't be listening.

… for forwarding failed connnection state errors
@Cartisim
Copy link
Contributor Author

Done ✅, Hopefully the test is sufficient. Thanks a bunch!

@Cartisim
Copy link
Contributor Author

Cartisim commented Oct 3, 2023

@Lukasa I am happy to do what I can to push this one through. Thanks for your work.

let channelError = error as? ChannelError
if channelError != .eof {
self.pipeline.fireErrorCaught(error)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you clean up the indentation here please?

// Step 2 is to fail all outstanding writes.
self.dropOutstandingWrites(error: error)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you remove the added whitespace here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It appears that on the main branch lines 262, 266, and 269 all have a single whitespace for those lines. Did I misunderstand what was needed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You added some spaces. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😳 Gotcha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.childChannelInitializer { channel in
return channel.eventLoop.makeSucceededVoidFuture()
}
.bind(host: "127.0.0.1", port: 8080)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we bind 0 instead? This can cause awkward conflicts if this port is in use somewhere else.

context.channel.write(ByteBuffer(data: Data()))
.whenFailure({ error in
listenerErrorPromise.succeed(error)
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this path should complete the promise: we're testing for this happening in errorcaught, not on the write promise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you cool with me just throwing the result of the write away?

Copy link
Collaborator

@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.

Nice one, thanks!

@Lukasa
Copy link
Collaborator

Lukasa commented Oct 24, 2023

Can you bring your branch up-to-date with main?

@Cartisim
Copy link
Contributor Author

@Lukasa
Thanks again for your hard work!

@Lukasa Lukasa added the semver/patch No public API change. label Oct 25, 2023
@Lukasa
Copy link
Collaborator

Lukasa commented Oct 25, 2023

@swift-server-bot test this please

@Lukasa Lukasa merged commit 16ca413 into apple:main Oct 25, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants