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

Add helpers Serve and ListenAndServe #355

Merged
merged 10 commits into from
Dec 1, 2022
Merged

Add helpers Serve and ListenAndServe #355

merged 10 commits into from
Dec 1, 2022

Conversation

hspaay
Copy link
Contributor

@hspaay hspaay commented Nov 30, 2022

First draft of a helper for Serve and ListenAndServe.
Using context for ListenAndServe as it is the only way to close it when needed.

@hspaay hspaay marked this pull request as ready for review November 30, 2022 00:50
@lthibault
Copy link
Collaborator

Just a quick thing: can you lowercase the file names?

Copy link
Collaborator

@lthibault lthibault left a comment

Choose a reason for hiding this comment

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

This is definitely on the right track. Don't be alarmed by the number of suggestions -- I'm just being thorough 🙂

And by the way, thanks for the contribution! 🙌

rpc/Serve.go Outdated Show resolved Hide resolved
rpc/Serve.go Outdated Show resolved Hide resolved
rpc/Serve.go Outdated Show resolved Hide resolved
rpc/Serve.go Outdated Show resolved Hide resolved
rpc/Serve.go Outdated Show resolved Hide resolved
rpc/Serve_test.go Outdated Show resolved Hide resolved
rpc/Serve_test.go Outdated Show resolved Hide resolved
rpc/Serve_test.go Outdated Show resolved Hide resolved
rpc/Serve_test.go Outdated Show resolved Hide resolved
rpc/Serve_test.go Outdated Show resolved Hide resolved
@hspaay
Copy link
Contributor Author

hspaay commented Nov 30, 2022

What is still missing is TLS support. I think that is a must for use in production.

Copy link
Contributor

@zenhack zenhack left a comment

Choose a reason for hiding this comment

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

A few things in-line. Additionally, it would be good to have a test or two that supplies a bootstrap interface and checks that it behaves as expected, incl. across connections (would catch the issue I pointed out in Serve()).

rpc/serve.go Outdated Show resolved Hide resolved
rpc/serve.go Outdated Show resolved Hide resolved
rpc/serve.go Outdated Show resolved Hide resolved
@zenhack
Copy link
Contributor

zenhack commented Nov 30, 2022 via email

@hspaay
Copy link
Contributor Author

hspaay commented Nov 30, 2022

If there's no actual effect then I'd prefer just using net.Listen; keep it simple.

After digging a bit deeper it is passed to a netFD.connect which does use it although how exactly I can't follow. I'm a bit concerned what happens if the context isn't closed after exit. ... since it isn't doing anything for our use-case lets stick to the regular listen.

@hspaay
Copy link
Contributor Author

hspaay commented Nov 30, 2022

A few things in-line. Additionally, it would be good to have a test or two that supplies a bootstrap interface and checks that it behaves as expected, incl. across connections (would catch the issue I pointed out in Serve()).

Indeed a good point. I'll expand the tests with using a bootstrap interface.

Added a test that uses Serve to transfer data.
rpc/serve_test.go Outdated Show resolved Hide resolved
rpc/serve_test.go Outdated Show resolved Hide resolved
Change API to take bootstrap client
instead of options
Added testcase to verify proper method handling using Serve.
Added verification of proper release of capabilities.
Update documentation to explain the functions release bootstrap client.
@hspaay hspaay requested review from lthibault and zenhack and removed request for lthibault December 1, 2022 01:31
Copy link
Contributor

@zenhack zenhack left a comment

Choose a reason for hiding this comment

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

A few more things in line.

rpc/rpc.go Outdated Show resolved Hide resolved
rpc/serve.go Outdated Show resolved Hide resolved
rpc/serve.go Outdated Show resolved Hide resolved
rpc/serve.go Outdated Show resolved Hide resolved
Copy link
Contributor

@zenhack zenhack left a comment

Choose a reason for hiding this comment

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

Almost there!

rpc/serve.go Outdated Show resolved Hide resolved
rpc/serve.go Outdated Show resolved Hide resolved
Remove the goroutine for handling the incoming connection as rpc.NewConn already does this.
Copy link
Collaborator

@lthibault lthibault left a comment

Choose a reason for hiding this comment

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

Almost done. The first PR always involves a bit of back-and-forth 🙂

rpc/rpc.go Outdated Show resolved Hide resolved
rpc/serve.go Outdated Show resolved Hide resolved
rpc/serve.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@lthibault lthibault left a comment

Choose a reason for hiding this comment

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

I think this is good to go! 🙂 🎉

@lthibault
Copy link
Collaborator

Test failure looks like #349. Re-running tests.

@lthibault lthibault merged commit 78b47e5 into capnproto:main Dec 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants