I have some queries about Connection.java. I would like to contact John Cowan. Do you think he would mind if I contacted him outside the forum? At his private email address? I think it is email@example.com, the address the forum shows.
For your information, I found some suspect code that publishes private items inproperly like getReceiver() while Connection works accross Component threads. Technically, the items involved "escape" proper synchronization, potentially causing all kinds of problems when accessed from other threads.
Kind regards, Guus.
(I keep forgetting that I can't answer your Discussion posts using Gmail!).
I am sure John wouldn't mind - however, any errors you found in Connection were probably introduced by me, as I have heavily modified his original, pristine, code, and I could very well have introduced synchronization errors, being a little weak on the concept at the best of times! So please make any changes you see fit, and, if you think I would understand what you did, you could try to educate me as well :-)
Also, there is a good chance that I have introduced the same errors into C#FBP, so I will endeavour to make the same changes in C#FBP as well.
Great stuff - I look forward to hearing what you come up with!
Thank you for your confidence: that is motivating!
I know nothing of C# though. Especially I do not know whether C# and Java work the same in terms of concurrency.
As discussed in our private email conversation, let me show the progress so far. I have found a few potentially improper publications that endanger threat safety, which are (I may miss some):
Component receiver and all other items
All items are declared without access modifier which grants them package visibility. This means that every class within the package is able to access these items directly, which is very bad. Even within thread confinement this would be a bad habit. Now, where we have multi threading, it is not just a bad habit, but has thread safety consequences: both visibility and state are compromised accross threads if access is not provided in a thread safe way.
So, in conclusion, all items are improperly published.
I will not change anything though until I have found the root cause and a means to remedy the IllegalMonitorStateException problem.
public Component getReceiver()
This is worse, as receiver is now effectively published to all who want to use it, even outside the package, without protecting the component itself.
There are several other methods that do the same.
I have not figured out whether the class Component and other classes are threadsafe at the moment. I definitely want to do that, after figuring out the IllegalMonitorStateException.
One of the things is, that John left the class Connection in a thread safe state, as he copied the method from a concurrency book by Doug Lea ("if you steal, steal from the best" was his quote!).
I did not contact John yet. When I do I want to be fairly sure of what is going on, as I do not want to waste his time.
This method is the culprit, that causes the IllegalMonitorStateException. Apparantly it tries to lock or unlock an item for which it does not own the lock. The item concerned is a component: receiver. What I do not understand fully is what this method is trying to accomplish.
Now Connection.indicateOneSenderClosed() is nicely synchronized on the inherent lock (this), thats ok. But ..... It tries to do direct locking and unlocking of the receiver component through the statements
and the statement
which means that it sets and unsets locks where eventually it tries to unlock a lock that it doesn't own (presumably component owns it).
Now this is the status so far. As soon as I know more about what is wrong and how to eliviate the problem I will let you know.
What would be helpful for me is if you could give me the highlights of what you were trying to accomplish when you did your changes to this object, generally what those changes were. I can see a lot of synchronization going on, but sometimes there is no synchronisation or using volatile or other thread protection mechanisms. If you could give me the highlights, that would be helpful in finding the problem.
I am very, very glad that someone who understands locking is finally looking at this - I have asked for this help on several occasions, with no luck!
Re your questions, I would have to rediscover what I was thinking, and I'm not even sure I can! And anyway I don't want to bias you. I do know goLock is new code and is the lock that controls (?) canGo. canGo is used when trying to decide whether to (re)activate a process (fire up a Component's run method). indicateOneSenderClosed implements the idea that you may have multiple output ports feeding one input port, so if one of the output ports closes, you have to check the others to see if they are all closed. If they are then the input port is now closed, and you can take appropriate action - most of that logic is in the InputStates internal class.
I don't think any of this was in John's original code, as I don't believe he had that concept, and the way I did it is probably not the best way - I was using a a blunt instrument approach - but all my test cases worked, so I figured it was good enough :-(
I look forward to hearing your feedback, and, as always, best regards,
BTW You can switch on tracing for the locks if you want - this may help...
Sorry - that should be a Component's execute() method, not run().
FYI: I am educating myself in the concurrency department, with help of java literature, specifically aimed at creating thread safe classes.
Reading your explanation I understand that you (and others) thoroughly changed John's software: of course, that is to be expected. However, maintaining thread safe classes, that stay thread safe is difficult and tedious.
We can make it easier by writing down the 3 components of a thread safe class, in the javadoc of the class, so future maintainers will understand how to modify:
You could add specific requirements that impact concurrency in any way, or may influence concurrency decisions.
Concurrency and performance
One of the liabilities of heavy synchronization is the performance penalty that comes from locking threads out, so they are waiting in line.
For this reason, people are always looking for ways to make thread safety as light as possible, by using a lighter to synchronize threads and still be thread safe. This does make thread safety more difficult to maintain, so good documentation is paramount.
We miss good concurrency documentation.
Lacking these, it is impossible to maintain a thread safe environment.
Best is to document thread safety, invariants and policies in the source code using Javadoc, so the docs will automatically show maintainers what they need to know.
Factoring out thread safety
At the moment sharing across threads is everywhere. Thats also makes it uncontrollable. One of the things I need to do, without changing functionality, is to factor out the thread safety area's and concentrate them in specific thread safe classes.
My plan is to refactor some of the code and restore thread safety for the fields and classes that need sharing across threads. I don't think it is easy, but I do think it is necessary.
The only think holding me back at the moment is that you have received no complaints about the frameworks concurrency or performance : have you?
Current threading documentation
Paul: do you have any documentation about the concurrency decisions you or John made early on in the project? That would help.
I apologize for the amount of chatter I give here, but I do think this needs some serious attention.
I solved the IllegalMonitorStateException by checking the owner first. Was actually pretty simple. Will check-in in a minute.
Now I want to start looking at the thread safety of classes, starting ofcourse from Network and Component. My intention is to add thread safety where I think it is necessary. This might mean some refactoring, without changing functionality. I promise to be as gentle as possible.
Do you agree?
P.S. If you want to I can confer with John first: see what he says.
Feel free to start making changes! I actually think you won't find many really fundamental errors <hubris!> but probably over-synchronization - and I was certainly careless about privates vs. publics. You may well find that just changing a bunch of publics to privates and maybe adding a few getters and setters will do the trick! My reasoning is that I have run so many heavy computing jobs, over several years, without a problem, that I suspect we are in pretty good shape! The one area that is probably a bit (maybe very!) flaky is the canGo logic, alluded to earlier. So go ahead - I'm really curious! All the best!
Thanks Paul, will start right away. It's good to hear you have been running heavy computing jobs with JavaFBP, because it is in the server area that the JVM does its heaviest optimization.
I will be careful what I alter. As always will let you know what I do through the forum and the commit messages.
"The only think holding me back at the moment is that you have received no complaints about the frameworks concurrency or performance : have you?"
Very little feedback, period! Sorry! And there seems to be no way to find out - short of burying a secret email function in Network! :-) Still, you would think that, if an app breaks after a couple of months of use, someone would at least send me an angry note!
hmmm, either that or they stop using JavaFBP. That might be more dangerous as mouth to mouth advertisement (Dutchism I am afraid) is all we have at the moment. Besides, concurrency problems are known not to be very reliable in occurring, so it might just happen haphazzardly.
En anglais, "word of mouth advertising"! OTOH, I suspect, if JavaFBP users have an app that they have put work into, and it only dies very occasionally, they wouldn't immediately ditch the whole app - they would either a) complain to the author (me), or b) try to figure out what is happening, or c) both. And I have left a pretty complete trail of breadcrumbs leading to me...
Fair enough. I will assume the concurrency trouble is limited in production.
Log in to post a comment.
Sign up for the SourceForge newsletter:
You seem to have CSS turned off.
Please don't fill out this field.