Crafting critical section

[Vith]

I was just glancing through the crafting code, and I have some doubts about ingredients availability check.

It goes like this:

1. The check is done with RecipeController$RecipeItemMaker.listItems(boolean remove) method, with the "remove" parameter indicating if ingredients shall be taken out of the inventory or not.
2. First time, it's run in RecipeItemMaker constructor ("remove" parameter = false), as one of verification steps.
3. Second time, it's run just before the success/fail check, in RecipeItemMaker.finishCrafting() method, this time with "remove" parameter = true.

Now, listItem() method looks like this:

Code: Select all

foreach (ingredient in ingredients)    check if there is enough items in player's inventoryif (remove)    remove items from player's inventory

Now, for me it looks like a classical critical section, that is not thread-safe. It is theoretically possible for a player (i.e. another server thread) to remove the ingredients just after the "foreach" part, but before the "remove" part. Of course that it *is* hard, but it is *not* impossible.

I believe introducing synchronization to the critical section mentioned is required. Please correct me if I'm wrong, I'm definitely not an L2j guru.

[JIV]

in theory yes but we dont make bank soft so in some places are synces ignored for performance benefit. This code is quite old maybe there would be a better way to handle it now (it dont check even destroyitem return value..). Also this will not be able with single packet executor.

[Vith]

By no means I consider this thing a critical issue or something. It's more just a tip to point your attention towards "proper" multi-threaded coding. I mean, from what I understand, L2j was created as an educational project, so this is like educational stuff. We all want to be *good* software devs (-;