Skip to content

Fix simulated recipe matching causing visible side effects - #5299

Open
Archengius wants to merge 2 commits into
GregTechCEu:1.20.1from
Archengius:fix-recipe-runner-side-effects
Open

Fix simulated recipe matching causing visible side effects#5299
Archengius wants to merge 2 commits into
GregTechCEu:1.20.1from
Archengius:fix-recipe-runner-side-effects

Conversation

@Archengius

Copy link
Copy Markdown

What

Fixes simulated recipe matching causing visible side effects, making it unsafe to run in parallel and violating semantics of simulated == true family of functions.

Implementation Details

Just gating of some functionality behind !simulated, clarifying contracts in IRecipeHandler interface in regards to purity and side effects, and changing ME hatch item/fluid list caching to avoid assigning the list before it is finished being built.

AI Usage

  • No AI driven tools were used for this pull request.
  • Yes AI driven tools were used for this pull request.

Claude Sonnet 5 was used to review the code.

Outcome

A certain subset of recipe search APIs is now safe to run in parallel.

How Was This Tested

Checked that recipe matching still works as expected.

Potential Compatibility Issues

None expected.

@Archengius
Archengius requested a review from a team as a code owner August 16, 2026 12:43
Comment thread src/main/java/com/gregtechceu/gtceu/api/capability/recipe/IRecipeHandler.java Outdated
listeners[i] = storages[i].getOnContentsChanged();
storages[i].setOnContentsChanged(() -> {});
Runnable[] listeners = null;
if (!simulate) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fairly certain this should always be done regardless of simulated status? Don't quote me on that, though.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It being done means the state is modified, which makes it unsafe to call from multiple threads. Why should it be done? In a simulated path, no change listeners will fire since no contents will be changed, and as such, that logic is not necessary.

for (int i = 0; i < storages.length; i++) {
storages[i].setOnContentsChanged(listeners[i]);
if (changed && action.execute()) listeners[i].run();
if (listeners != null && !simulate) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

Runnable listener = storage.getOnContentsChanged();
storage.setOnContentsChanged(() -> {});
Runnable listener = null;
if (!simulate) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto


storage.setOnContentsChanged(listener);
if (changed && !simulate) listener.run();
if (!simulate) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants