Make TcpIncoming public

[acl-cqc]

Motivation

We call the serve_with_shutdown method of tonic::transport::server::Router passing a local socket address. If the server is unable to bind the address, that returns an Err (quite quickly). If it is able to bind the address, it returns Ok only much later, when serving is complete.

We'd like to know when(/whether) the server has successfully bound the address, i.e. as soon as we know that bind isn't going to fail, and when a client can begin connecting to it (avoiding any race condition).

Solution

The above can be done using the serve_with_incoming_shutdown method instead, passing a TcpIncoming struct - exactly as serve_with_shutdown does (it first builds the TcpIncoming, which binds the socket and may fail; and then passes that to the serve_with_incoming... method). By performing these two steps itself, the client can detect success/failure of constructing the TcpIncoming before calling the long-running serve... method.

Hence, the "solution" is merely to make the TcpIncoming struct public - it is currently pub(crate) within tonic::transport::server::incoming - and to pub use rather than use it from tonic::transport::server.

Tests

I note the contribution guidelines state "Bug fixes and new features should include tests. I'm not clear whether this constitutes a bug fix or a new feature. Whilst I can imagine a test that started a server and then tried to create a second TcpIncoming on the same socket-address (looking for a failure), that test sounds like a much larger change than the rest of the PR. So, guidance as to what tests might be appropriate would be appreciated :-), thank you!

[LucioFranco]

Hi! Thanks for the contribution, I think we should probably add a test related to what you want and in addition, if we are making this public we need to add docs. FYI the plan is to remove the transport feature soon (not in the upcoming release).

[acl-cqc]

Hi @LucioFranco and thanks for the quick response!

I think your last point that transport is planned for removal may be critical - is this #31 ?

[acl-cqc]

Hi @LucioFranco, I've put in

• a few docs (to the extent that I understand what's going on here)
• a unit test that TcpIncoming has the behaviour we want - unfortunately I don't see any way to verify that the nodelay/keepalive params are respected in AddrIncoming
• A no_run doc-test that shows its usefulness - this could be smaller if we revert 0aaa6dc for the simpler version before

I did also write a larger test of creating+connecting to a Server/Router, we could have that if we wanted but I think that might be overkill (about 50 lines in integration_tests). How's this?

[LucioFranco]

LGTM! Thanks!