Download the latest KolMafia build here.
Every new revision posted within the hour.
New EXE builds every Monday.
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.
- 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? :-)
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.
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!
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)
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.
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.
"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.