Repository navigation
Fix BlockIterator callback lifecycle issue. - #6794
Conversation
|
Hi, can someone reviev my code? |
| hRadius = Math.max(hRadius, horizontalRadius); | ||
| vRadius = Math.max(vRadius, verticalRadius); | ||
|
|
||
| if (processing) pendingCallbacks.add(callback); |
There was a problem hiding this comment.
module logic is inherently synchronous, and a cross-thread write to ReferenceArrayList can end up throwing a ConcurrentModificationException
if anything this should be
if (processing) throw new IllegalStateException();but i think keeping the method as it was previously is fine as modules being synchronous goes without saying
| try { | ||
| callback.run(); | ||
| } catch (RuntimeException e) { | ||
| MeteorClient.LOG.error("Error while executing a BlockIterator after callback.", e); | ||
| } |
There was a problem hiding this comment.
never try/catch arbitrary module code, failing-fast is important for getting issues reported on the tracker
| for (int x = px - scanHRadius; x <= px + scanHRadius; x++) { | ||
| for (int z = pz - scanHRadius; z <= pz + scanHRadius; z++) { | ||
| for (int y = Math.max(mc.level.getMinY(), py - scanVRadius); y <= py + scanVRadius; y++) { | ||
| if (callbacks.isEmpty()) break scan; |
There was a problem hiding this comment.
prefer having the scan be a separate method over using label breaks
|
|
||
| if (disableCurrent) { | ||
| iterator.remove(); | ||
| completedCallbacks.add(callback); |
There was a problem hiding this comment.
the completedCallbacks list is redundant, you can just release the callback back into the pool here
| List<Runnable> callbacksToRun = new ReferenceArrayList<>(afterCallbacks); | ||
| afterCallbacks.clear(); |
There was a problem hiding this comment.
uselesss thread safety, module code should always be synchronous
|
Thanks for your feedback. I've made the necessary changes based on your suggestions. I've also implemented the recommended improvements to make the code cleaner and callback handling more straightforward. I appreciate your feedback. |
|
please dont let claude loose on classes when a 3 line pr would have sufficed |
I manually reverted the changes to the class you mentioned. Is my code okay now, or should I reduce the PR to just three lines? |
Type of change
Description
This PR improves callback handling in BlockIterator to prevent unnecessary block scanning and ensure reliable callback cleanup.
Related issues
Im not find any.
How Has This Been Tested?
BUILD SUCCESSFUL in 40s
Game starts without errors
Checklist: