connection.go : concurrent map writes

[piano-man]

The Connection struct seems to use normal maps and not concurrency safe maps. This leads to concurrent map write errors sometimes.

[mpiraux]

That's something I had on sight for a long time. I would like to avoid introducing safe maps when no write conflict currently exist. Maybe that makes extending QUIC-Tracker less easy, thoughts ?Currently there are the following maps.

CryptoStates           map[EncryptionLevel]*CryptoState
PacketNumber           map[PNSpace]PacketNumber
LargestPNsReceived     map[PNSpace]PacketNumber
LargestPNsAcknowledged map[PNSpace]PacketNumber
AckQueue               map[PNSpace][]PacketNumber

CryptoStates is written to by the connection, the TLS agent and the key update scenario. The write conflic between the agent and the scenario is avoided, as the scenario waits for the handshake to complete.

PacketNumber has several write conflicts I think. Every time a packet structure is created using the corresponding constructor, a packet number is assigned. One way to avoid this would be to choose a packet number right before sending. That would also remove the little bit of code that decrements a packet number when an empty packet is generated. But the ability to choose a particular packet number in a scenario should be retained, despite no scenarios using this currently.

LargestPNsReceived is written to by the Parsing agent only.

LargestPNsAcknowledged is written to by the Recovery agent only.

AckQueue could be moved to ACK agent, as no one is accessing it.

[piano-man]

Thanks for the detailed response :)

It does seem to be the case that the concurrent write errors are occurring because of the PacketNumber map. So just making that concurrency safe should fix the problem.

Assigning a number before sending the packet(probably in the EncodeAndEncrypt function ?) does seem more sophisticated but couldn't multiple agents call the DoSendPacket or EncodeAndEncrypt function though ? In that case, wouldn't a safe map still be needed ?
I doubt that not making the other maps safe would affect extensibility of QUIC-Tracker unless someone wants to create a weird scenario that alters the CryptoStates map after the handshake(highly unlikely I guess)

Ability to choose a packet number in a scenario would probably be important if a scenario is testing something like this if I'm not wrong :- "A QUIC endpoint MUST NOT reuse a packet number within the same packet number space in one connection. If the packet number for sending reaches 2^62 - 1, the sender MUST close the connection without sending a CONNECTION_CLOSE frame or any further packets; an endpoint MAY send a Stateless Reset (Section 10.4) in response to further packets that it receives."

[mpiraux]

Errors also happen when a read and a write occurs simultaneously. This is frequently the case for CryptoStates.