Issue description:

failure to get capability keeps trying infinitely 1.6gb server log file and rising this would also crash from any mod that adds large energy storage. serverside it keeps player from loading it kicking them but in singeplayer it will crash you.

Steps to reproduce:

1.place mekanism ultimate cabless
2.add Overloaded-1.11.2-0.0.24 and place infinity capacitor
3.connect ultimate cables to infinity capacitor

Version (make sure you are on the latest version before reporting):

Forge: 2315
**Mekanism-1.11.2-9.3.2.307
Other relevant version:

If a (crash)log is relevant for this issue, link it here: (It's almost always relevant)

https://pastebin.com/AcagQiCV side note if this is fixed in 308 or 309 it would be nice if that was on curse.

Its not a failure to get, I treat getCapability as non-pure so its causing the cable to think I have changed my state and ask again..

Looks like you are using it to see if it canRecieve.

This is an issue of overloaded. They are marking an entity as dirty in the get capability method, which should not happen there.
Please report to them.

Note to the developer of overloaded:
Remove the markDirty() at https://github.com/CJ-MC-Mods/Overloaded/blob/master/src/main/java/com/cjm721/overloaded/block/tile/infinity/TileInfiniteCapacitor.java#L47

#minecraftforge can't make up their mind on if getCapability should be pure or not (50/50). I am going to change my code for a more defensive style but getCapability should be considered not-pure.

There is no reason a mod cannot use get capability calls to do other actions that can cause side effects, for example a cost for every getCap call to incentive users to use mods that generally will perform better. If get capability was meant to be pure there would be no reason for has capability, as you could just call getCap == null instead.

For the FE API there are pure methods in the Cap returned but that is not true for all Caps and get Capability in itself should have no clue about the inner workings of that other system, just that it knows that it has been used and the data it would need to save may change. The only other way is to have a call back from that cap to notify the data holder that there has been a change which is making for needless complexity and coupling (which I have now done as a defensive measure, but does not mean this is still not an issue).

Add on top of that your current system triggers a call for every update of a block instead of just adding to a set queue of updates that should be done on next tick. For any blocks that cause a massive amount of updates (large chests, or large amount of FE packets on a cable) you are calling get capability a needless amount of times.

I will submit a pull request to make Mekanism a bit more defensive against this kind of stack overflows. Shouldn't be necessary, but it's a little change with little to none by-effects.

There is no reason a mod cannot use get capability calls to do other actions that can cause side effects

Can not != should not. Many mods get capabilities in neighborChanged - e.g. determining if a connection to the changed block should be rendered. To do this in a tick would not be performant. As Forge itself is the metric, where in Forge does a state change occur during getCapability?

as you could just call getCap == null instead.

You can indeed do this!

get Capability in itself should have no clue about the inner workings of that other system

getCapability should only return an object that can manipulate the object it is called on. The object you return should definitely know about the inner workings and how to map those to standard funtionality.

The only other way is to have a call back from that cap to notify the data holder that there has been a change

Marking your tile dirty when the capability it retrieved is not a guarantee that any changes made by the user of the cap will get saved. What if it gets cached and used after your tile is saved? You should markDirty only when your state is actually changed.

For any blocks that cause a massive amount of updates (large chests, or large amount of FE packets on a cable)

None of those things trigger a neighborChange. This method is called when a neighbouring Block or BlockState changes.

Also, if a Tile were to call getCapability on your object every tick (for instance, sending FE packets every tick as often happens) then you are causing Forge to iterate through the map of loaded chunks every tick, which can't be good for a server's performance.

Causing your own update to happen on neighborChanged is just asking for chain reactions even outside of the capabilities system (any sort of BUD).

As Forge itself is the metric, where in Forge does it state that get capability is pure? If something is not defined you should make the least amount of assumptions possible.

Also we seem to have different definitions of what a capability is. I do not see it as a subset of anything it is attached to; rather it is its own independent system. You can make it a subset of its caller but it is not designed to be that way or it would of been doing using interfaces instead.

If you are caching a cap outside of the same tick get Cap is called you are breaking the capability system. Lex has stated that you when you use getCap the value returned should not be stored for more then a tick. (Follows same rules as references to TileEntities.)

Every time you add or remove from a system does your state not change? If you have 100 things that all interact with that same system each one will have a separate interaction in that same tick and the system will need to mark itself as dirty. The number of dirty calls will not change if it was done in the cap or on the TE in that scenario.

Minecraft's code is atrocious. markDirty has two separate meanings

1. By the given documentation: Its just suppose to make sure the world knows that the chunk it is in needs to be saved before unloading.
2. By its actual code: It does #1 then also notifies the blocks around it.

Those two actions should not be tied together, but nonetheless they are and we have to deal with it.

Also to change dirty it does not iterate through the map, it uses the hash function so its 0(1).

Causing your own update to happen on neighborChanged is just asking for chain reactions even outside of the capabilities system (any sort of BUD).

Except I didn't actually say a change WOULD be made in that function; after getting a capability you can query the capability - hence why it is redundant to call markDirty on your block.

or it would of been doing using interfaces instead

A capability implementation object IS an interface / implements an interface. It is an object with a standard API to manipulate the Tile/ItemStack/World in a defined way.

If you are caching a cap outside of the same tick get Cap is called you are breaking the capability system. Lex has stated that you when you use getCap the value returned should not be stored for more then a tick. (Follows same rules as references to TileEntities.)

Where exactly is this? I can't see it anywhere in the documentation. I'm not actually advocating caching Caps either.

You are updating your state on neighbor change just you don't call markDirty because you don't need to save it. Like I said markDirty has two separate functions. I don't want to cause the onNeighborChange because not all caps that need to be saved need to trigger external updates (Ender Chest), but the TE does know that it is in charge of saving data of that Cap.

I was talking about interfaces on the TE itself. Capability object is an implementation of an interface on non-specified object.

Lex stated it on #minecraftforge several times when it has come up (but still has not added it to any thing offical, thats forge docs for you).

In all reality the original issue that this conversation started from has also been discussed in #minecraftforge and it got mixed responses of the intent / purity of get capability. It came down to that the Capability system is not well designed or defined and updates in get capability are discouraged because of current use (like yours) but not because anything stated in design or docs, just how people used it unknowingly. I changed my code to be more defensive and you said you are going to change your code to be more defensive. I agree that is not the best solution but neither of us have the ability to cause the best solution (rework of minecraft / forge)

Mekanism uses the capability to check if a cable should connect to a block. The performance friendly way is to check when a block changes (it marks it self dirty) as capabilities could change every thick.
If the block changes it state on a get of the capability we end up with an infinite loop. This could result in a stack overflows with multiple mods

The part that is the issue is the getCapability, the hasCapability is expected to be pure. Which you do check first but then go on to see if the Capability returned by getCapability canRecieve.

@cjm721 no, you are calling markDirty for absolutely no reason - you're not changing any TE data. getCapability should not change state at all!

You are causing a state change (in the chunk, hence the update neighbours call) in get capability. Sure it does mean your tile gets saved, but it does so even when unnecessary. Which then causes issues with other mods because you didn't think about the consequences (I.e read-only use of the cap).

And if it's not in the docs, don't assume someone, somewhere, won't do it. I sure didn't know he has said it, and I know of a mod in 90 percent of 1.10 packs that does cache caps.

Oh I thought about the consequences then assumed as by the structure that has was pure and get was not (as discussed before, most get methods are pure, by convention, but then there be no reason for has so in this case get was a bad name choice). Minecraft and forge do not follow professional conventions which makes efficient programming for anything they touch very hard. Also most mod developers have never had any formal teaching either which just makes the problems even worse. Having to write defensive programming is never a good thing but not like we have many other choices.

As for that mod in 1.10.2 you might want to tell them that they are using potentially invalid objects (really easy way to get a dupe bug if you can put it on a chunk border and cause some flash chunk reloads (same tick unload then load).

What exactly do you mean by "pure"?

Also the reason for hasCapability in addition to get is likely because of how itemstack capabilities work - there is no way to use caps on items without everyone making their own item stack class (and how do you know who to call to create it), so getCapability must create an object in that case, thus a boolean test exists, but is not required to be used

There are different levels of purity but in general they mean the amount of side effects a function has (memory changes). So if a method is pure it has no observable side effects (if it is strongly pure it has no side effects at all). So when I say getCap is not pure I mean it can have effects outside the given stack (Like changing things in the world).

This is where intent in design becomes a very hard question. Item Stack capabilities are a much better example of what I was trying to get at. Item Stacks have no concept of notification of updates, they also always overwrite save even if nothing has changed (unlike chunks) so there is no need to state that you need to save. So from that perspective Caps should not trigger any updates, but from a use logic standpoint they are part of the state so for TileEntity which do have a notification of update they should be triggered but that means the implementation of the Cap System has to know what is using it which is very strong coupling (which is bad), or the TileEntity can just assume the state will be changed and there is no coupling from the Cap to the TE. If forge was to come out and give an official word on it we would not be having this discussion, but since there is no official it comes down to community use (as getting forge to change the API for Caps seems rather impossible). I would rather the community follow the same professional coding standards that I do at work because they make sense and less likely to cause errors while also reducing the need for defensive programming. Also would give something like a rule-book to point to alleviate discussions like this.

From a way to fix this on my end I did some weak coupling by making a new interface that is just a change observer for the Caps and made anything that hasCap implement it so that it may known when notify of changes. From a performance perspective the only time it is better is when there are many getCap calls that actually never do anything, but it is worse in all other cases (standard use).

So when I say getCap is not pure I mean it can have effects outside the given stack (Like changing things in the world).

Here I would say the getCapability method itself is intended to be pure (judging by example implementation in docs & implementations inside Forge), however that does not mean the methods exposed by the returned Interface implementer are pure - some of them quite obviously change state. I think assuming state has changed just because someone looked at you is lazy programming.

So from that perspective Caps should not trigger any updates

Do you mean getCapabilities itself, or the object returned from getCapabilities? The object returned is definitely intended to trigger updates, eg. an IItemHandler's insert/extract. It is the responsibility of the caller of the IItemHandler to not use a non-simulation (impure) method in a place where recursion would occur.

but that means the implementation of the Cap System has to know what is using it

I disagree (perhaps only semantically), it is only necessary for the cap implementation to know how to signal it wishes to be saved when and only when its state is actually changed. This seems to be what you have done with your IDataUpdate. You could also do this in a way that does not depend on the TileEntity, aside from passing in an identifier, by using WorldSavedData, or perhaps WorldEvent.Save

Also from a performance perspective, I don't understand how you think it will worsen performance, as you are actually causing less world saves.

I encourage you to contribute to the Forge docs by submitting them a pull request. The whole section on the readthedocs site about ore dictionary was written by a non team member it would seem

After skimming through this it's pretty evident Mekanism isn't doing anything wrong here.