Skip to content

No more shown finished background tasks#11574

Merged
calixtus merged 8 commits into
mainfrom
demo-background
Aug 4, 2024
Merged

No more shown finished background tasks#11574
calixtus merged 8 commits into
mainfrom
demo-background

Conversation

@koppor

@koppor koppor commented Aug 4, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

JabRef collected all run tasks. This was confusing. This PR fixes this.

Mandatory checks

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@calixtus calixtus added component: ui status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Aug 4, 2024
@calixtus calixtus enabled auto-merge August 4, 2024 15:23
@calixtus calixtus added this pull request to the merge queue Aug 4, 2024
@github-actions

github-actions Bot commented Aug 4, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

The build for this PR is no longer available. Please visit https://builds.jabref.org/main/ for the latest build.

Merged via the queue into main with commit 2c8fc01 Aug 4, 2024
@calixtus calixtus deleted the demo-background branch August 4, 2024 15:46
@koppor

koppor commented Aug 4, 2024

Copy link
Copy Markdown
Member Author

Follow-up depends on controlsfx/controlsfx#1559

@koppor koppor mentioned this pull request Aug 4, 2024
Closed
@InAnYan InAnYan mentioned this pull request Aug 5, 2024
Closed
1 task
@LoayGhreeb

LoayGhreeb commented Sep 4, 2024

Copy link
Copy Markdown
Member

@koppor, @calixtus
This PR causes an exception that prevents JabRef from shutting down properly. When I close JabRef while a background task is running, Will asked either to wait for the task to finish or cancel the task and shutdown. If I choose to cancel, an exception is thrown, JabRef window closes, but IntelliJ shows that JabRef is still running.

Exception 
Caused by: java.util.NoSuchElementException: java.lang.IndexOutOfBoundsException
	at java.base/java.util.AbstractList$Itr.next(AbstractList.java:379)
	at java.base/java.util.Iterator.forEachRemaining(Iterator.java:133)
	at java.base/java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1939)
	at java.base/java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:762)
	at org.jabref@100.0.0/org.jabref.gui.util.UiTaskExecutor.shutdown(UiTaskExecutor.java:131)
	at org.jabref@100.0.0/org.jabref.gui.JabRefGUI.shutdownThreadPools(JabRefGUI.java:376)
	at org.jabref@100.0.0/org.jabref.gui.JabRefGUI.stop(JabRefGUI.java:366)
	at javafx.graphics@22.0.2/com.sun.javafx.application.LauncherImpl.lambda$launchApplication1$10(LauncherImpl.java:858)
	at javafx.graphics@22.0.2/com.sun.javafx.application.PlatformImpl.lambda$runAndWait$12(PlatformImpl.java:483)
	at javafx.graphics@22.0.2/com.sun.javafx.application.PlatformImpl.lambda$runLater$10(PlatformImpl.java:456)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:400)
	at javafx.graphics@22.0.2/com.sun.javafx.application.PlatformImpl.lambda$runLater$11(PlatformImpl.java:455)
	at javafx.graphics@22.0.2/com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:95)
	at javafx.graphics@22.0.2/com.sun.glass.ui.win.WinApplication._runLoop(Native Method)
	at javafx.graphics@22.0.2/com.sun.glass.ui.win.WinApplication.lambda$runLoop$3(WinApplication.java:184)
	... 1 more
Caused by: java.lang.IndexOutOfBoundsException
	at javafx.base@22.0.2/javafx.collections.transformation.FilteredList.get(FilteredList.java:169)
	at com.tobiasdiez.easybind@2.2.1-SNAPSHOT/com.tobiasdiez.easybind.MappedList.get(MappedList.java:31)
	at java.base/java.util.AbstractList$Itr.next(AbstractList.java:373)
	... 15 more

I tried to revert this commit, and the issue was resolved.

To reproduce the issue:

  • Checkout the Lucene branch (Lucene search #11542).
  • Open a library with PDFs.
  • If the files are already indexed, use "Rebuild Index."
  • Try to close JabRef before indexing is finished.
  • Click Yes when prompted. image

@koppor koppor mentioned this pull request Sep 4, 2024
Closed
LoayGhreeb
LoayGhreeb reviewed Sep 24, 2024
View reviewed changes
Comment thread src/main/java/org/jabref/gui/util/UiTaskExecutor.java
StateManager stateManager = Injector.instantiateModelOrService(StateManager.class);
if (stateManager != null) {
stateManager.getBackgroundTasks().stream().filter(task -> !task.isDone()).forEach(Task::cancel);
stateManager.getRunningBackgroundTasks().stream().forEach(Task::cancel);

@LoayGhreeb LoayGhreeb Sep 24, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The issue was that canceling a task removes items from the runningBackgroundTasks list since it's a filtered list. Removing items during iteration causes an IndexOutOfBoundsException.

Reverting this line will fix the issue.
I will reopen the PR.

@LoayGhreeb LoayGhreeb restored the demo-background branch September 24, 2024 05:56
@LoayGhreeb LoayGhreeb mentioned this pull request Sep 24, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LoayGhreeb LoayGhreeb LoayGhreeb left review comments

@calixtus calixtus calixtus approved these changes

Assignees

No one assigned

Labels

component: ui status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@koppor @LoayGhreeb @calixtus