Support for v3 protocol #89
Conversation
RogerZhongAWS
force-pushed
the
multipleTCP
branch
from
f059a03 to
77eac4d
Compare
RogerZhongAWS
force-pushed
the
multipleTCP
branch
2 times, most recently
from
07e9253 to
49b7674
Compare
RogerZhongAWS
force-pushed
the
multipleTCP
branch
from
80f983d to
f2f08e7
Compare
add accept parameter
RogerZhongAWS
force-pushed
the
multipleTCP
branch
from
8a2501f to
cdc0ac7
Compare
RogerZhongAWS
changed the title
marcoemorais-aws
left a comment
marcoemorais-aws
left a comment
There was a problem hiding this comment.
Read through the protocol doc and very quickly went through the code. Some comments on both, but mostly looks good. Want to spend a little more time with the code in my next pass.
| async_send_message(tac, outgoing_message); | ||
| } | ||
|
|
||
| void tcp_adapter_proxy::async_send_stream_reset(tcp_adapter_context &tac, std::string const & service_id, uint32_t const & connection_id) |
There was a problem hiding this comment.
Minor. What is the meaning of connection_id for StreamReset? It seems we ignore the parameter and the intended behavior is that receipt of a StreamReset will immediately close all open connections for the service. Recommend to drop the parameter and either (1) not set on the outgoing message -- (although with proto3 this has less meaning since there is no support for HasField); or (2) the protocol seems to describe some logic that any connection id of 0 is interpreted as id of 1, make that explicit and pass 1 instead of 0.
| } | ||
| tcp_server::pointer server = tac.serviceId_to_tcp_server_map[service_id]; | ||
|
|
||
| uint32_t new_connection_id = ++server->highest_connection_id; |
There was a problem hiding this comment.
I see that highest_connection_id is monotonically increasing and will overflow to 0 at 4B. Since 0 is invalid connection_id, do we have to implement wraparound to 1 instead of 0?
I know that MAX_ACTIVE_CONNECTIONS limits the number of active connections, but it would be possible in some rare case to start-reset enough connections to hit the maximum. If that happens, then without wraparound to 1, we could have a broken connection mapped to 0 or (worse) duplicate connections mapped to 1.
There was a problem hiding this comment.
I originally made connection id an unsigned long, but after a bit of deliberation with the team, we agreed to keep it as a 32 bit int. The tunnel lifetime imposes a strict time limit, after which the localproxy would shut down. This would require almost 100000 connections to be created per second to overflow within 12 hours
* implementation for simultaneous tcp connections
3 participants
Motivation
Modifications
Change summary
Revision diff summary
If there is more than one revision, please explain what has been changed since the last revision.
Testing
Is your change tested? If not, please justify the reason.
Please list your testing steps and test results.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.