[#3675] Fix livelock issue in MpscLinkedQueue #3684
Conversation
|
@trustin @Scottmitch please review... @gsoltis please check if this fix the problem you reported in #3675 |
|
|
||
| @Override | ||
| public Object[] toArray() { | ||
| return toList(16).toArray(); |
There was a problem hiding this comment.
Does the runtime type of toList(..) have to be ArrayList<>? Can you just change the signature of toList(..) to accept a List<..> since it is private? That way we don't have to duplicate the 16 is the default size logic here.
There was a problem hiding this comment.
@Scottmitch sorry but I not understand what you are proposing.. IF I understood you right you may want to have toList accept a List<...>, I just not see why this is better then what we have now. Can you explain ?
There was a problem hiding this comment.
Where did 16 come from? Is this a value we have explicitly decided is good...or are we just using it because it is also the default value of some common collection implementations in Java? If we have justification why this makes sense...then never mind me (but add a comment :) ). Otherwise if we don't have any rational for the size and just choose it because that is what default implementations choose then lets just let the implementation choose the default (i.e. new ArrayList<>()) and change the parameter from a int to a List<>.
There was a problem hiding this comment.
@Scottmitch The 16 came from the mighty @normanmaurer =P Joking aside I just thought it may not be a bad fit as queues often have some items in there. That said I not care too much about this method as we not use any of these methods that call it in our code-base.
There was a problem hiding this comment.
we not use any of these methods that call it in our code-base.
Sounds like the mighty @normanmaurer is setting a trap for our users :)
There was a problem hiding this comment.
honestly it is in the internal package... That said if you have a better idea shoot 👍
There was a problem hiding this comment.
Agreed not a big deal...but why have magic numbers when you can not have them...
Change this:
private List<E> toList(int initialCapacity) {
..
}
@Override
public Object[] toArray() {
return toList(16).toArray();
}to this:
private List<E> toList(List<E> elements) {
..
}
private List<E> toList(int initialCapacity) {
return toList(new ArrayList<E>(initialCapacity));
}
private List<E> toList() {
return toList(new ArrayList<E>());
}
@Override
public Object[] toArray() {
return toList().toArray();
}then also search and replace the other magic 16 number floating around in this file
|
@netkins build |
|
@gsoltis do you have any updates? |
|
Unfortunately, no updates yet from my end. The only thing I can add is that On Mon, Apr 27, 2015 at 12:18 PM, Norman Maurer notifications@github.com
|
|
Ok thanks!
|
| @@ -197,22 +211,16 @@ public boolean contains(Object o) { | |||
| @Override | |||
| public Iterator<E> iterator() { | |||
| return new Iterator<E>() { | |||
| private MpscLinkedQueueNode<E> node = peekNode(); | |||
| private final Iterator<E> it = toList(16).iterator(); | |||
There was a problem hiding this comment.
This would be a candidate for https://github.com/netty/netty/pull/3684/files#r29030165
|
@trustin please review as well |
|
@trustin ping :) |
|
@normanmaurer Looks good to me. I wonder if the upstream already has a better fix for this issue. Did you have a chance to talk with him? |
|
@trustin that said as far as I know the upstream queue impl does not reuse "nodes" as head so I think we need to be extra careful, and that is why I think we should use my change ;) WDYT ? |
|
@normanmaurer OK. No worries then. |
|
@trustin cool :) Cherry-pick then ? |
|
@trustin btw let me also add an integer overflow check just to be 100% safe |
normanmaurer
force-pushed
the
live_lock_fix
branch
from
cc2ebcf
to
6f59041
Compare
Motivation: All read operations should be safe to execute from multiple threads which was not the case and so could produce a livelock. Modifications: Modify methods so these are safe to be called from multiple threads. Result: No more livelock.
normanmaurer
force-pushed
the
live_lock_fix
branch
from
6f59041
to
9550195
Compare
Motivation:
All read operations should be safe to execute from multiple threads which was not the case and so could produce a livelock.
Modifications:
Modify methods so these are safe to be called from multiple threads.
Result:
No more livelock.