Break out RecvCryptoHandshake into a separate function that just returns an
error code and message, and doesn't terminate the connection. This is more
suitable for the situations where are initializing / accepting a connection.
Also fix bug trying to send signals (especially the connect request signal)
before we have our crypto ready.
P4:6856380,6856401
- Add m_bWaitForInitialRoutingReady, and make sure we only do this check once.
- Remember when we sent our last signal, and add some basic rate limiting.
- Fix bug clearing the m_bAppConnectHandshakePacketsInRSVP flag. It should NOT
be set while we are in the connected state!
- Handle reliable RTO better. Make sure we never schedule a *send* based on a
current RTO! We should only schedule a *check* based on on the RTO, and then
only send if the RTO has actually expired!
- Move the code to send the connect request message into the generic signal
population code. This way, if we send a signal during this state for any
other reason, even if it's outside of the specific connect request state
machine, we will always include the connect request.
P4:6856358
Don't try to do it lazily at any other time.
Added some asserts and clarified comments.
This fixes a bug if the first call you make is IterateGenericEditableConfigValues.
That could have been solved with a narrower fix, but I realized the lazy initialization
was really just not useful and doing it this way makes more sense.
P4:6854435,6854450
Previously we were keeping the reliable segments that had been sent in two
non-overlapping std::maps: in-flight, and ready-for-retry.
Also, each in-flight packet would remember the reliable segments it contained
as begin/end pairs. When a packet was acked or nacked, we would need to lookup
the range in the maps.
The new method puts all segments that have been sent in a linked list, and we
track the status of each segment. The in-flight packets hold a reference to
them, so there is no more need to lookup. Likewise, the retry list is just a
list of referneces into that list.
This is *much* more memory efficient:
- The in-flight references are each one short, instead of a 64-bit begin/end
pair.
- And we use CUtlLinkedList, which stores the data in a contiguous array,
instead of std::map, which makes tons of tiny allocations.
I went in a cirlce on the design for the data structures for this at least 4
or 5 times, probably spending 10+ hours on it until I finally got something I
didn't totally hate. There was always a bubble under the carpet that I could
move around but not elimiate. In this design, the bubble is one place where
we do do some linear searching: When we need to retry a segment, we have to
find the proper place to insert it into the retry queue. This is because
reliable serialization assumes that the segments will be presented in order
of increasing stream position for a given lane. However, at least this list
should usually be short.
In the typical case of relatively little packet loss and need for reliable
retry, this code is much, MUCH more efficient than the previous code with
memory (and probably CPU -- although I haven't measured it!). I believe
that to fix the remaining bubble under the carpet I need to change the
reliable retry list to be a map, ordered by some segment serial number.
But I don't want to do that with a std::map. So I think I might bring
back Valve's ordered map CUtlMap, which uses integers (shorts if you want)
as stable handles to the elements, and uses contiguous memory allocation.
That would be good for this case, and also optimize a few of the other maps
as well. But I think this code is good enough for now.
Also: added per-lane stats for number of bytes pending in various states.
P4:6834549
Using my wacky len() function, which I realize everybody hates but me
and Guido can Rossum. Actually he probably would hate it, too.
P4:6834536,6834538
Separate out "trivially copyable" and "relocatable" (which I guess is
the same as "trivially movable".
By default relocatable will be the same as trivially copyable,
however since this is not in the std::namespace, I am explicitly
creating an opportunity to tag your type as being relocatable.
Because this makes it possible to do stuff GCC doesn't think is
safe, we'll need to turn off their warnings.
Implement packet decode for the lane select frame. I have not executed all
this one single time! (But I have been making sure I haven't broken the
single-lane case.) If I have not written any bugs, the the feature almost
done!
Remaining work:
- Write test harness, make sure it does actually work...
- Bump versions, deal with old peers who don't understand lanes, etc.
Also, add STEAMNETWORKINGSOCKETS_MAX_LANES.
- Add some optimizations if STEAMNETWORKINGSOCKETS_MAX_LANES is small.
In particular, STEAMNETWORKINGSOCKETS_MAX_LANES=1 disables support,
and all the code is compiled out.
- Also this can be used to limit the amount of work a malicious sender
can make a receiver do.
P4:6819330,6819343
Changed my mind on the lane select command. Encoding this as a delta
from the previously selected lane really only gives a tiny benefit,
a single byte *if* several stars align. And even then, that savings
could not be predicted in advance. Furthermore, doing this required
sorting the lanes, which is just bad. So basically it added a bunch
of runtime complication for extremely dubious benefit to the wire
encoding. Just a bad tradeoff all around.
So, new design: the lane select command always *sets* the current lane.
Much, much simpler for encoding, and almost no loss to wire efficiency
except for a byte or two in some edge cases.
I did keep one small optimization, which is that lane 0, if present,
will always be encoded first.
Still a work in progress.
P4:6818528
- Updated wire protocol document
- Implement encoding for multiple lanes.
- Added function declaration to ISteamNetworkingSockets with rather copious
documentation. (But left it commented out for now, until it all works).
Remaining work:
- Implement decoding
- Bump versions, deal with old peers who don't understand lanes.
- Add tests
P4:6819091,6819098
- Added a system for strict priority classes, in addition to the "weight"
system. The combination of these two makes for a very flexible system,
while also hopefully being easy to understand.
- Each lane has its own message number sequence. This has several
advantages:
- It makes it clear that the order of messages between lanes is
not guaranteed
- It greatly simplifies packet encoding/decoding. (Going to make some
more progress on that next.)
(Based on discussions with jeffreyh@valvesoftware.com. Thanks!)
P4:6818689,6818692
Context: https://twitter.com/ZPostFacto/status/1445748202926342144
NOTE: This is *not* currently useful in the opensource code, so
STEAMNETWORKINGSOCKETS_ENABLE_DUALWIFI is never defined. It depends
on the sender negotiating with the receiver that a secondary source
IP:port address will be associated with the same connection, and that
has not been implemented for ordinary UDP connections at this time.
(On Steam it is part of the SDR protocol.) When I do that work, I
probably will do it in conjunction with some other planned work to
support doing all UDP connections on the same socket, and using the
connection ID for multiplexing instead of IP:port tuples. This
strategy not only is more efficient, due to fewer sockets being
used, but it also makes it very easy to support the peer roaming to
a different IP:port without the connection dropping, which can happen
in mobile environments and is a characteristic of QUIC.
Added SegmentCollector template.
The work we have to do to organize the segments will depend on what
features the app is using. If they are using reliabile messages
and multiple lanes, it is the most work.
P4:6806943
Backtracking a bit and taking a different approach for fast paths
when the app isn't using reliable or multiple lanes.
- Break out most of the body of SNP_SendPacket into
SNP_SerializePacketInternal. It is a template function, with fast paths
for when the app is not using our reliability layer or multiple lanes.
- Delete CSteamNetworkConnectionBase::SNP_EncodeSegment and just expand it
back inline where it was. I don't think that I will be calling this from
multiple places with this new template fast path method.
p4:6806913
Add a few more things to SNPPacketSerializeHelper that make sense:
- m_cbRemainingForSegments is used in several places across boundaries that
I am going to want to break up into functions.
- Store the inflight packet we are building up directly in the helper.
- And because the inflight packet has a timestamp, that is where the current
timestamp will be, we don't need a separate m_usecNow
- And because the inflight packet has has a transport, change references to
the pTransport function argument to use this, which should hopefully mean
that the compiler can use my copy, instead of saving off a copy of its own.
Also, added a note that if we fail a low-level send, we are currently leaking
messages, possibly catastrophically, if they are reliable! Will come back and
fix this later.
P4: 6806902
Some refactoring to make it easier to create fast paths and duplicate some code:
- Move the check for shifting virtual time to
SSNPSenderState::CheckShiftVirtualTime
- Put several pieces of context into a new struct, SNPPacketSerializeHelper.
This reduced the number of arguments I need to pass to some functions.
- Broke out segment encoding into CSteamNetworkConnectionBase::SNP_EncodeSegment.
Later I will be calling this from more than one place.
P4:6806890
Lanes are different messages queues with different priority levels.
Work in progress. This change just adds some data structures, but
the actual wire encoding/decoding not yet implemented.
See also:
- Issue #95
- https://twitter.com/Mokosha/status/1444009029987041281
P4:6806862
Move check to see if we are idle to be inside the (rarely true) if()
branch where we use it. This could become a bit more important in
the future if SNP_BHasAnyBufferedRecvData() becomes more expensive
when lanes are added.
P4:6806855
- Move all the functions to be declared inline in the header, so that we can
optimize out the constant pointer-to-member. I briefly considered making
this a template parameter (and got it working), but decided that this was
an esoteric-enough C++ language feature that I risked it not working on
some compiler that mattered. Also, I believe that most compilers will be
able to inline this and the code would been identical.
- Delete the class SSNPSendMessageList, which had some "optimized" functions
I dont' think this is going to be useful. push_back() was not any faster
than LinkToQueueTail(), which is now inline. pop_front() is no longer useful
because of some changes I am about to make with "lanes" (priority groups /
channels), which make the assumptions it relies on no longer valid. And
this was a really minor speedup anyway, so it's no huge loss even ignoring
the changes to lanes.
P4:6806839
So, if you send to one of your local identities using any of the available
methods, it will recognize this and use the optimized internal loopback.
- CMessagesEndPoint::BHandleNewIncomingConnection needs to be able to accept
base class connection type, can no longer assume it's a P2P connection
- CSteamNetworkConnectionPipe::CreateLoopbackConnection now uses the fast
path unless there is a messages endpoint session. (Previously it would
use the slow path if you did a connection-oriented connect call to a
local listen socket.)
- Handle clients connecting from their global FakeIP
- Fix assert if a CSteamNetworkConnectionPipe encounters a "problem detected
locally". There actually is such a case! If we attempt a connection and
the application doesn't "accept" it in time, the client side will time out.
P4:6761717
I need this in order to get loopback connections to work with CMessagesEndPoints
Also delete some code that was obviously duplicated for no reason
p4:6761716
This is called the "slow path", although it's really still pretty efficient.
We need this if we're going to make it possible for CMessagesEndPointSession
to use loopback.
P4:6761714
When creating loopback connections, be more deliberate about setting the
remote identity at the right time, matching the callbacks that would be
triggered for ordinary connections as closely as possible.
(In fact, previously the remote identity was totally blank in the first
callback, which was a bug.)
Also started blocking out a special case that we'll need for FakeIP
P4:6761708
Fletcher Dunn
Sam Lantinga
Levent
Cyprien AUGIER