Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A cancel button for puzzle building #140

Open
cutelyaware opened this issue Sep 14, 2020 · 0 comments
Open

A cancel button for puzzle building #140

cutelyaware opened this issue Sep 14, 2020 · 0 comments

Comments

@cutelyaware
Copy link
Owner

cutelyaware commented Sep 14, 2020

With Don's support of lovely new puzzles comes a greater need to be able to cancel puzzle creation of the truly humongous ones. I was able to find a reasonable way to support this in the UI, and to make it start to work, but debugging all the possible race conditions is beyond what either of us is willing to do to finish the task. Here is the patch containing my attempt: magiccube4d-cancel.zip

Here is Don's feedback:

(1) it looks like your patch changes that inBackground param you're talking about from false to true in one or maybe two calls during startup (one from MC4DSwing.java, maybe only if there's a log file, and one from inside the PuzzleManager ctor). I don't know if that was intentional, but I think maybe those should remain false? The fact that they are now true makes it so that the puzzle creation happens in the SwingWorker thread, and doesn't complete by the time PipelineUtils needs it to draw the first frame, so that's why you're getting null stuff there. If I change them back to false, that seems to fix the exceptions on startup.

(2) You'll need to keep track of whether you cancelled the PolytopePuzzleDescription ctor, and, if so, don't use the constructed PolytopePuzzleDescription object, since it's bad.
Actually, I'm thinking it would probably be simpler and less error-prone if, instead of having your updateProgress() callback return false on cancellation, just make it throw some exception of your choice instead; that'll throw all the way out of the PolytopePuzzleDescription ctor and back up into your calling code, and your catch block can handle it however you like, and there will be no bad object for you to have to go out of your way to avoid using. So, I think maybe I can change all the callbacks to return void instead of bools, that'll simply things and make this kind of hard-to-diagnose bug in the caller impossible.

(3) Even before your patch, it looked to me like the background thread creation of the puzzle is likely generally racy, as I think you've pointed out in your comments or original commit description (I forget which). In particular this:

             public Void doInBackground() {
                 PuzzleDescription newPuzzle = buildPuzzle(schlafli, lengthString, this);
                 if(newPuzzle != null) {
                     succeeded = true;
                     puzzleDescription = newPuzzle;
                     faceColors = ColorUtils.generateVisuallyDistinctColors(puzzleDescription.nFaces(), .7f, .1f);
                     resetPuzzleStateNoEvent();
                 }
                 return null;
             }

So it looks like the background thread is changing several pieces of state here, non-atomically, at the same time that the foreground thread (EDT) is using all that state. While it might be getting by, to some extent, by luck, I'm not surprised if it's crashy and unpredictable, and so it might be difficult to get very far with further enhancements like this Cancel button.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant