Opened 9 months ago

Closed 7 months ago

#2699 closed defect (fixed)

Fix FragmentHandler

Reported by: jogger Owned by: zzz
Priority: major Milestone: 0.9.46
Component: router/general Version: 0.9.45
Keywords: Cc: Zlatin Balevsky
Parent Tickets: Sensitive: no

Description

I read about possible concurrency problems here and found that the fields of FragmentedMessage? are not volatile. Syncing should not be sufficient. Issues should only arise with SSU and messages ≥ 3 fragments running through multiple threads.

Also _cache seems unused.

Message generation produces lots of unnecessary locking as it it misses use of the double check idiom.

Subtickets

Change History (6)

comment:1 Changed 9 months ago by zzz

Component: router/transportrouter/general

Not a lot of detail here. FragmentHandler? sync issues were a major issue in 2009 that took several tries to track down and fix, perhaps that's what you were reading about? Not aware of any current problems and didn't find any with a quick scan through the code.

You're suggesting that FragmentedMessage? fields should be volatile?

What do you mean by "message generation" is that on the fragmentation or reassembly side?

_cache is used

I don't see anything major, need more clues please.
_completed and _failed should be AtomicIntegers?, thats all I see so far

comment:2 Changed 9 months ago by jogger

a) Example: FragmentHandler? syncs 3 times on 3 fragment msg. Thread A creates the message and receives first fragment. Thread B copies the message data from A, receives last fragment. Thread A receives 2nd fragment. Since _lastReceived and _highFragmentNum from Thread B may be not visible to Thread A because they are not volatile, message gets lost. Reported as incomplete with _highFragmentNum=2.

b) _cache in FragmentHandler? is only released, that is unused in my eyes.

c) as noted elsewhere: Lots of syncing could be avoided in FragmentHandler? using

            msg = _fragmentedMessages.get(Integer.valueOf(messageId));
            if (msg == null) {
                synchronized (_fragmentedMessages) {
                    msg = _fragmentedMessages.get(Integer.valueOf(messageId));
                    if (msg == null) {
                        msg = new FragmentedMessage(_context, messageId);

In case you are interested, I have converted FragmentHandler/FragmentedMessage? to lockless parallel reception of fragments, could provide code.

comment:3 Changed 8 months ago by zzz

Cc: Zlatin Balevsky added

a) as I understand it, synching in FragmentHandler? should be sufficient, cc'ing zab for advice

b) the 1K cache is used several other places, so items released in FragmentHandler? will be used elsewhere

c) Is it worth the trouble? have any measurements of any performance benefit?

comment:4 Changed 8 months ago by zzz

AtomicIntegers? and other very minor cleanups in 4d980bd6259a7604dad58eb5051a99d7c3383bf0 to be 0.9.45-4

comment:5 Changed 8 months ago by Zlatin Balevsky

Since _lastReceived and _highFragmentNum from Thread B may be not visible to Thread A because they are not volatile

They will be visible because thread A will only attempt to read them after acquiring the lock on the message object, which will force a memory barrier from thread B to thread A.

From what I can see so far there is no need for additional locking.

comment:6 Changed 7 months ago by zzz

Milestone: undecided0.9.46
Resolution: fixed
Status: newclosed

Looked at this one more time, I agree with zab, pretty sure the locking is correct as is, with the atomic integer fix from comment 4 above.

Note: See TracTickets for help on using tickets.