SSU review

[Zlatin Balevsky]

trac ticket associated with ​http://zzz.i2p/topics/1198

follow that post for updates

[Zlatin Balevsky]

Some comments that didn't make it to the forum post becaus zzz.i2p went down:

OutboundMessageState?:

• _pushCount and _nextSendTime are thread safe
• _maxSends is not - written by pusher, read in rtt & window calculation outside of lock
• _expiration is not - written during initialize, read from pusher thread and elsewhere
• _peer is not - written in initialize and add, read from udp pusher and udp reader threads w/o lock
• _startedOn is not - written in initialize(), read through getLifeTime() outside of lock in rtt & window calculation
• _fragmentSize is not - written in locked_shouldSend, read outside of lock in rtt & window calculation
• _fragmentSends is not - same as above, also contents modified in OutBoundMessageState?.acked by reader thread and OutboundMessageState?.push by pusher thread; read outside of lock in OutboundMessageFragments?.preparePackets

[Zlatin Balevsky]

These comments also apply to :

• _message,_messageId : set in initialize() and read from a whole bunch of threads including pusher and reader

Replying to zab:

Some comments that didn't make it to the forum post becaus zzz.i2p went down:

OutboundMessageState?:

• _pushCount and _nextSendTime are thread safe
• _maxSends is not - written by pusher, read in rtt & window calculation outside of lock
• _expiration is not - written during initialize, read from pusher thread and elsewhere
• _peer is not - written in initialize and add, read from udp pusher and udp reader threads w/o lock
• _startedOn is not - written in initialize(), read through getLifeTime() outside of lock in rtt & window calculation
• _fragmentSize is not - written in locked_shouldSend, read outside of lock in rtt & window calculation
• _fragmentSends is not - same as above, also contents modified in OutBoundMessageState?.acked by reader thread and OutboundMessageState?.push by pusher thread; read outside of lock in OutboundMessageFragments?.preparePackets

[Zlatin Balevsky]

PeerState?:

• add - if _dead is true, the OutboundMessageState? does not release resources. See below.
• finishMessages :
  • should check for _dead before checking _outboundMessages *_outboundMessages.isEmpty - why is it ok for the short-circuit to not be syhchronized? - PacketPusher? thread
• getOutboundMessageCount() - definitely must be synchronized* it's called PacketHandler?.handler thread
• are you sure you don't need hashCode() and equals() methods?
• dropOutbound() : _retransmitter = null should be moved inside the sync block few lines below
• locked_shouldSend() : when is it ok for state.getMessage() to return null?
• _concurrentMessagesActive,_consecutiveFailedMessages,_concurrentMessagesAllowed : these are NOT safe even though they are volatile. ++ and — are not atomic operations so you need more coarse locking around all rtt & window computations. Don't worry about speed in this case, a single monitor is faster than multiple volatile reads and writes to different memory locations. (Has to do with cpu cache line state)

[Zlatin Balevsky]

UDPReceiver :

• _keepRunning : volatile
• _socketChanged: doesn't seem to be used but if it were to be used it should be volatile
• updateListeningPort() if it were ever used, it would block indefinitely since it tries to lock the Runner instance which is locked during an unbounded blocking receive() call.
• Runner.run() : you don't want to continue the main loop in case of IOException. If your datagram socket gets closed or otherwise damaged you will be re-trying to read and re-throwing IOExceptions in busy loop. This means you also don't want to throw an IOException if the packet is too large. This is a good use case for a custom exception type.
• _inboundQueue : you don't want this queue to be unbounded because this opens up many fun DOS vectors. Come up with some reasonable bounds and log warnings if you hit them (i.e. offer() returns false) . But don't log every time because that opens other fun DOS vectors.
• _inboundQueue : the LinkedBlockingQueue? provides concurrency only between offer() and poll(). If you have multiple processor threads, they will still bump in each other while trying to poll().