Connection.java

Developers
2013-11-10
2013-11-17
  • Guus Bonnema

    Guus Bonnema - 2013-11-10

    Hi Paul,

    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 cowan@ccil.org, 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.

     
  • J. Paul Morrison

    (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!

    Regards,

    Paul

     
  • Guus Bonnema

    Guus Bonnema - 2013-11-10

    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.

    Regards, Guus.

     
  • Guus Bonnema

    Guus Bonnema - 2013-11-15

    Hi Paul,

    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.

    General threadsafety
    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.

    Connection.indicateOneSenderClosed
    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

          getReceiver().goLock.lockInterruptibly();
    

    and the statement

          getReceiver().goLock.unlock();
    

    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).

    Status
    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.

    Kind regards, Guus.

     
    Last edit: Guus Bonnema 2013-11-15
  • J. Paul Morrison

    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,

    Paul

    BTW You can switch on tracing for the locks if you want - this may help...

     
  • J. Paul Morrison

    Sorry - that should be a Component's execute() method, not run().

     
  • Guus Bonnema

    Guus Bonnema - 2013-11-15

    Hi Paul,

    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.

    Thread safety
    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:

    1. What is the state of a thread safe class (all published fields)?
    2. What are the invariants of the fields and dependencies between the fields?
    3. What is the synchronization policy of the class?

    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.

    JavaFBP
    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.

    Kind regards, Guus.

     
    Last edit: Guus Bonnema 2013-11-15
  • Guus Bonnema

    Guus Bonnema - 2013-11-15

    Paul,

    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.

    Kind regards, Guus.

     
  • J. Paul Morrison

    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!

     
  • Guus Bonnema

    Guus Bonnema - 2013-11-16

    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.

    Kind regards, Guus.

     
  • J. Paul Morrison

    "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!

     
  • Guus Bonnema

    Guus Bonnema - 2013-11-16

    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.

     
  • J. Paul Morrison

    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...

     
  • Guus Bonnema

    Guus Bonnema - 2013-11-17

    Fair enough. I will assume the concurrency trouble is limited in production.

    For now.

     

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks