Results 1 to 10 of 10

Thread: 18985: Synchronize two maps in hopes that an intermittent NPE was caused by unsafe th

  1. #1
    Feed Reader RSS Bot's Avatar
    Join Date
    Jul 2009
    Posts
    11,478

    RSS 18985: Synchronize two maps in hopes that an intermittent NPE was caused by unsafe th

    Synchronize two maps in hopes that an intermittent NPE was caused by unsafe thread access. If it works credit xKiv @ Kolmafia.us. If it doesn't I'll take the blame.

    by jaadams5 on 2018-11-09 21:27:49

    M /src/net/sourceforge/kolmafia/textui/DataFileCache.java (view) (diff)
    Download the latest KolMafia build here.
    Every new revision posted within the hour.
    New EXE builds every Monday.

  2. #2
    Senior Member
    Join Date
    Apr 2009
    Posts
    1,820

    Default

    I am not sure synchronizing the map themselves instead of the (public) methods that use them is correct. You are still left with possible race conditions when two threads both find that a file isn't there, and both try to fill it in. Although in this case you might just "only" be wasting the effort necessary to read the file twice, with the same end result.

  3. #3
    Developer fronobulax's Avatar
    Join Date
    Feb 2009
    Location
    Central Virginia, USA
    Posts
    4,182

    Default

    I am not sure synchronizing the map themselves instead of the (public) methods that use them is correct. You are still left with possible race conditions when two threads both find that a file isn't there, and both try to fill it in. Although in this case you might just "only" be wasting the effort necessary to read the file twice, with the same end result.
    Originally Posted by xKiv View Post
    I believe the following is true about a synchronizedMap.

    • Every read/write operation needs to acquire lock.


    Therefore I don't need to worry about the getters and setters because they will be implicitly synchronized.

    We can get in trouble if something tries to iterate across the map but if there is an instance of that in the current codebase I missed it.

    I agree that two threads could be looking for the same file and concurrency could result in the file being read twice, instead of the cache being used, but I don't see that creating a data integrity problem.

    I am having difficulty constructing a scenario where two threads could block each other instead of one getting the lock but my lack of imagination doesn't mean there is no such scenario.

    If there is a potential problem, a ConcurrentHashMap is an alternative as well as any one of several "roll your own" thread safe cache implementations.

    Since my track record in this area is somewhat abysmal, we can continue to discuss, decide to revert or live with this until a problem is actually reported. User == Beta Tester, right? :-)
    You just vehemently agreed with me
    Originally Posted by Veracity View Post
    I agree with frono.
    Originally Posted by Veracity View Post

  4. #4
    Developer
    Join Date
    Aug 2009
    Posts
    2,820

    Default

    I am having difficulty constructing a scenario where two threads could block each other instead of one getting the lock but my lack of imagination doesn't mean there is no such scenario.
    Originally Posted by fronobulax View Post
    Deadlocks only arise if you have multiple mutexes, and you can acquire them simultaneously (and, in particular, in an inconsistent order).

    For instance, suppose you have mutex A, B, and threads 1, 2.

    Thread 1 tries to acquire A, then B.
    Thread 2 tries to acquire B, then A.

    (In particular, this is the "circular wait" condition, which can be avoided by imposing an order on mutex acquisition)

    If all accesses to mutexes are implicit via map methods (get / set / whatever else), I think it's incredibly unlikely that the standard library would a) have shared mutexes that b) are acquired in an inconsistent order, leading to deadlocks. And I'd expect each method to release all of its acquired mutexes before returning.

  5. #5
    Senior Member zarqon's Avatar
    Join Date
    Nov 2007
    Location
    Seoul, Korea
    Posts
    3,593

    Default

    Deadlocks only arise if you have multiple mutexes
    Originally Posted by heeheehee View Post
    I would very much like if the plural of "mutex" were "mutices".
    Sig by JakAtk
    My scripts: Prefref Plus | Skillref Plus | One-Click Wossna | Om*****st (??) | Psychose-a-Matic | RandBot
    Combat suite: Best Between Battle | Mercenary Mood | SmartStasis | BatMan | BatMan RE
    For script authors: ASH Wiki | ZLib | BatBrain | CLI Links | Drag-n-Drop Inventories | CanAdv | Script Registry | Map Manager
    If you appreciate my work, help me become BAT KING OF THE WORLD! Thanks to all donators!

  6. #6
    Senior Member
    Join Date
    Apr 2009
    Posts
    1,820

    Default

    ...
    but I don't see that creating a data integrity problem.
    ...
    I am having difficulty constructing a scenario ...
    ...
    Originally Posted by fronobulax View Post
    Or you could guarantee thread-safety of your method by synchronizing the whole thing. Which you can't guarantee by using only thread-safe components anyway.
    You shouldn't predict scenarios and patch-defend only against what you can imagine. You should expect that you *cannot* imagine everything, and use the proper defense.
    (And thread safety is about making sure that your method is doing what it is designed to do, no matter what happens in other threads. That's not just data integrity in objects under your direct control, but also any side-effects. What if that extra time spent double-reading throws off timing somewhere? What if reading that file just one more time triggers some kind of alarm? Or reads something completely different, because the "file" is actually a hardware device reporting sensor data (and you are unknowingly losing half of the data)? You have no idea where your data is coming from and what happens when you read it ... )

    (not to mention that you are wasting time acquiring the lock potentially twice in a scenario where you will be acquiring it at least once no matter what)

  7. #7
    Developer fronobulax's Avatar
    Join Date
    Feb 2009
    Location
    Central Virginia, USA
    Posts
    4,182

    Default

    Or you could guarantee thread-safety of your method by synchronizing the whole thing. Which you can't guarantee by using only thread-safe components anyway.
    You shouldn't predict scenarios and patch-defend only against what you can imagine. You should expect that you *cannot* imagine everything, and use the proper defense.
    (And thread safety is about making sure that your method is doing what it is designed to do, no matter what happens in other threads. That's not just data integrity in objects under your direct control, but also any side-effects. What if that extra time spent double-reading throws off timing somewhere? What if reading that file just one more time triggers some kind of alarm? Or reads something completely different, because the "file" is actually a hardware device reporting sensor data (and you are unknowingly losing half of the data)? You have no idea where your data is coming from and what happens when you read it ... )

    (not to mention that you are wasting time acquiring the lock potentially twice in a scenario where you will be acquiring it at least once no matter what)
    Originally Posted by xKiv View Post
    I paid my dues writing mission critical code so I understand the issues and techniques. I also understand that writing bullet proof code has a cost. As a volunteer supporting a client for a game, I am willing to make some compromises. Others may accuse me of being lazy and I will plead guilty. If someone tells me this makes things worse for KoLmafia I will gladly revert it. If someone believes this implementation will not solve the original problem then changes are in order and should be discussed.

    But, I have always believed it is easier to make a working program fast than make a fast program work. In that spirit if a bullet proof solution were required, I would say that caching the file contents to speed things up was a premature optimization and I would rip out the caching code. Side effect would be that if a file is used more than once and changed between uses then the changed version would get used.
    You just vehemently agreed with me
    Originally Posted by Veracity View Post
    I agree with frono.
    Originally Posted by Veracity View Post

  8. #8
    Senior Member
    Join Date
    Apr 2009
    Posts
    1,820

    Default

    But, I have always believed it is easier to make a working program fast than make a fast program work.
    Originally Posted by fronobulax View Post
    This is true. And
    1) I think you didn't do that. I believe hat tthe "working program fast" way was to just synchronize the entire method and be done.
    2) That's the part that needs to be synchronized to prevent *any* suprises from other threads anyway, no matter what.
    3) I have been personally in too many situations where it lead to code that had to be ripped out months later and replaced with something completely different, at cost much higher than just doing it right in the first place. Sometimes with the same justification that they denied when *I* used it.
    4) This isn't about "fast", it's about "working" (or, perhaps, "correct at all"). It's easier to make a program work correctly by being too cautious (and then perhaps optimize later), than to make it subtly incorrect and then have to debug it much later (especially if the debugging is left to completely different people).


    The current code change should definitely prevent the NPEs though, so the reported bug is fixed. I don't see it reemerging without more code changes.

  9. #9
    Developer
    Join Date
    Aug 2009
    Posts
    2,820

    Default

    I would very much like if the plural of "mutex" were "mutices".
    Originally Posted by zarqon View Post
    (This is the same sort of logic that brought you boxen. Mutex is a shortened form of "mutual exclusion object", and as such isn't of Latin origin.)

  10. #10
    Developer Veracity's Avatar
    Join Date
    Mar 2006
    Location
    The Unseelie Court
    Posts
    11,447

    Default

    "Boxen" really pisses me off. It is oh-so-cute from people who have no understanding of - or care about - etymology.

    "Ox" is an Old English word. It is a rare noun form which pluralizes as "oxen". There are essentially no other words which have made it from Old English to Modern English with that noun form.

    "Box" (and "Vax", the DEC computer) are not examples of such. So, pluralizing them with "en" is just Oh So Precious that I can't stand it.
    Speaking as somebody who studied Old English when I was going for my Masters in Medieval Studies.

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •