@skiwi Large sites (SO) I honestly don't care so much about. On Code Review though, the auto comments script I'm using is my best friend (next to monkeys and CRitters of course).
I am just reading in Effective Java, that (in boldface, which is very sparsely used): Therefore, it is impossible to exclude concurrent activity from a concurrent collection; locking it will have no effect but to slow the program.
Look what we have here:
@Override
@SuppressWarnings("unchecked")
public <T> void registerListener(final Class<T> eventClass, final Consumer<? extends T> eventListener) {
Objects.requireNonNull(eventClass);
Objects.requireNonNull(eventListener);
if (!classConstraint.isAssignableFrom(eventClass)) {
return;
}
synchronized(eventMapping) {
eventMapping.putIfAbsent(eventClass, new HashSet<>());
eventMapping.get(eventClass).add(new ConsumerEventHandler((Consumer<Object>)eventListener));
Does that not directly violate what is said there?
Or perhaps the synchronized should be on this (an EventBus instance) rather than on the eventMapping
Don't synchronize on this, that can cause problems as you're then leaking the monitor object (this), which allows other parts of your code to synchronize on that. Ask @Mat'sMug for an example of what horrible things (deadlocks) can happen from synchronizing on a public object.
@rolfl If there are too many, then you can work on them slowly or no more. I'm just focusing on some of the tags that are missing a small number of questions.
@skiwi No, thread safety is not limited to the classes you use. .... and, using Concurrent* does not mean it is safe, in fact, often the worst concurrency problems are masked by incomplete, or incorrect use of java.util.concurrent.
@nhgrif 5 people of the 11 visitors have watched the video; it would seem you don't speak for the entir internet. Please can you keep ur comments relevant to finding a solution to the question asked. Additionally, your responses so far have not helped. I ask you kindly to stop voting down all my questions. The past two posts i've made you voted down. Why? Why not be supportive of new developers trying to find a solution? Are you above us n00bs now that youve got it all nailed? Your not helping the website by being so negative, so why not join the community spirit & post a solution to my issue? — Dai Lafing6 mins ago
Just because you use a class that is natively thread-safe, does not mean that your code is safe.
For example, the JDOM code I posted earlier, in the JDOM 1.x version, even though there will never be any corruption in the static data structures, there is in fact a synchronization bug.... and it could be significant.
Uhm. Should any code ever produce results that are contrary to expectations but not documentation or vice versa? Or really... should expectations and documentation ever be at odds?
I decided to roll out my own EventBus system which is intended to be thread-safe.
Hence a review should focus extra on thread safety apart from all regular concerns.
The EventBus can work in two ways:
You can register events and listeners directly on the EventBus.
You can the methods, of a sp...
Why would it be...? (Thinking aloud) The classConstraint is a constant after construction, the event is an argument (thread safe), and then it points to a ConcurrentHashMap, is what I think
The right solution is to not use synchronization on eventMapping
@SimonAndréForsberg No, that does not solve the synchronization problem in the iteration.
(and will likely lead to concurrentmodificationexceptions
The problem with the inside-HashSet is that you have one thread doing slow iteration, and another thread doing short inserts.
You need to ensure that you have exclusive access to the HashSet for the entire iteration.
Synchronizing on the CncurrentHashMap, will ensure that, but, then you lose almost all advantages for having parallel threads... you may as well be single-thread.
Also, synchronizing on a Concurrent* object is just plain ugly, and means that you have a design problem.
Give me a few minutes, I'll propose an alternative.
@rolfl You mean that the forEach call is not synchronized? Are you sure that call is not synchronized/thread-safe? Considering it's Java 8, I would assume that they had made it thread-safe.
No, it is not synchronized (and it is potentially parallel, in multiple threads, and, what is/would it synchronized on... the stream? the class, the instance, the what?)
btw, You really need to post a more detailed version of the full code as an answer
I'd suppose it would be synchronize on a private lock to the synchronizedset?
The Collections.synchronizedSet mandates that you should synchronize on the site while doing normal iteration (or enhanced for-loop)
However
SynchronizedSet<E> extends SynchronizedCollection<E>, which has:
// Override default methods in Collection
@Override
public void forEach(Consumer<? super E> consumer) {
synchronized (mutex) {c.forEach(consumer);}
}
Therefore, it is thread-safe?
Even though a stream() is not synchronized
(In SynchronizedCollection<E>)
// Override default methods in Collection
@Override
public void forEach(Consumer<? super E> consumer) {
synchronized (mutex) {c.forEach(consumer);}
}
@Override
public boolean removeIf(Predicate<? super E> filter) {
synchronized (mutex) {return c.removeIf(filter);}
}
@Override
public Spliterator<E> spliterator() {
return c.spliterator(); // Must be manually synched by user!
}
@Override
public Stream<E> stream() {
return c.stream(); // Must be manually synched by user!
}
@Override
I'd say that Collections.synchronizedSet even smells...
Pretty nice question here (which I've just answered) that was almost pushed off the front page. Anything else to say on this, @Corbin and @JerryCoffin?
As we know, iterating over a concurrent collection is not thread safe by default, so one cannot use:
Set<E> set = Collections.synchronizedSet(new HashSet<>());
//fill with data
for (E e : set) {
process(e);
}
This happens as data may be added during iteration, because there is no exclusive...
The synchronization in the code is in some places overly broad, and in others, it is absent where it is needed.
synchronizing on eventMapping in your registerListenersOfObject method means that only one thread can be accessing the eventMapping instance at any one time. This defeats using the Con...
I'm sure designers of languages like Java or C# knew issues related to existence of null references (see Are null references really a bad thing?). Also implementing an option type isn't really much more complex than null references.
Why did they decide to include it anyway? I'm sure lack of null...
I decided to roll out my own EventBus system which is intended to be thread-safe.
Hence a review should focus extra on thread safety apart from all regular concerns.
The EventBus can work in two ways:
You can register events and listeners directly on the EventBus.
You can the methods, of a sp...
As we know, iterating over a concurrent collection is not thread safe by default, so one cannot use:
Set<E> set = Collections.synchronizedSet(new HashSet<>());
//fill with data
for (E e : set) {
process(e);
}
This happens as data may be added during iteration, because there is no exclusive...
