Opened 9 years ago
Last modified 7 years ago
#657 assigned enhancement
SSU review
Reported by: | Zlatin Balevsky | Owned by: | |
---|---|---|---|
Priority: | maintenance | Milestone: | |
Component: | router/transport | Version: | 0.9 |
Keywords: | SSU | Cc: | |
Parent Tickets: | Sensitive: | no |
Description
trac ticket associated with http://zzz.i2p/topics/1198
follow that post for updates
Subtickets
Change History (5)
comment:1 follow-up: 2 Changed 9 years ago by
comment:2 Changed 9 years ago by
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:
- _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
comment:3 Changed 9 years ago by
- 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)
comment:4 Changed 9 years ago by
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().
comment:5 Changed 8 years ago by
Owner: | zzz deleted |
---|---|
Status: | new → assigned |
Note: See
TracTickets for help on using
tickets.
Some comments that didn't make it to the forum post becaus zzz.i2p went down:
OutboundMessageState?: