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.

Sign up for GitHub

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

Jump to bottom

Abbreviation of a list of entries cause two uncaught exceptions #12634

Open
2 tasks done
marwanemad07 opened this issue Mar 6, 2025 · 14 comments · May be fixed by #12686
Open
2 tasks done

Abbreviation of a list of entries cause two uncaught exceptions #12634

marwanemad07 opened this issue Mar 6, 2025 · 14 comments · May be fixed by #12686
Labels
component: journal abbreviations good third issue

Comments

@marwanemad07
Copy link
Contributor

marwanemad07 commented Mar 6, 2025 •
edited
Loading

JabRef version

Latest development branch build (please note build date below)

Operating system

Windows

Details on version and operating system

Windows 11

Checked with the latest development build (copy version output from About dialog)

  • I made a backup of my libraries before testing the latest development version.
  • I have tested the latest development version and the problem persists

Steps to reproduce the behaviour

  1. Create a new library.
  2. Use "Web Search" to search for something (for example, “SW”).
  3. Select all resulting entries and import them into your library.
  4. Select all the imported entries
  5. From the "Quality" tab, abbreviate the journal names to "dotless." If the error does not appear, abbreviate them to "shortUnique" and repeat these steps a couple of times. The exceptions do not occur consistently.

Explanation:
I traced the code and found that the issue occurs because a CompoundEdit object, which is not thread-safe, is accessed concurrently. In the AbbreviationAction class, the abbreviate method creates multiple Callable tasks that use NamedCompound instance. When I changed the code to iterate over the entries in a regular loop (instead of collecting tasks in threads) the errors no longer occurred.

Appendix

Exceptions:
Note: The first exception always happens, and the second one happens rarely.

Log File 
ERROR: Uncaught exception occurred in Thread[#248,JabRef CachedThreadPool,5,main]: java.lang.IllegalStateException: Called endChange before beginChange
	at javafx.base@23.0.2/javafx.collections.ListChangeBuilder.endChange(ListChangeBuilder.java:538)
	at javafx.base@23.0.2/javafx.collections.ObservableListBase.endChange(ObservableListBase.java:210)
	at javafx.base@23.0.2/com.sun.javafx.collections.ObservableListWrapper.access$200(ObservableListWrapper.java:46)
	at javafx.base@23.0.2/com.sun.javafx.collections.ObservableListWrapper$1$1.invalidated(ObservableListWrapper.java:74)
	at javafx.base@23.0.2/com.sun.javafx.collections.MapListenerHelper$Generic.fireValueChangedEvent(MapListenerHelper.java:318)
	at javafx.base@23.0.2/com.sun.javafx.collections.MapListenerHelper.fireValueChangedEvent(MapListenerHelper.java:70)
	at javafx.base@23.0.2/com.sun.javafx.collections.ObservableMapWrapper.callObservers(ObservableMapWrapper.java:115)
	at javafx.base@23.0.2/com.sun.javafx.collections.ObservableMapWrapper.put(ObservableMapWrapper.java:173)
	at org.jabref@100.0.0/org.jabref.model.entry.BibEntry.setField(BibEntry.java:640)
	at org.jabref@100.0.0/org.jabref.model.entry.BibEntry.setField(BibEntry.java:658)
	at org.jabref@100.0.0/org.jabref.gui.journals.UndoableAbbreviator.abbreviate(UndoableAbbreviator.java:60)
	at org.jabref@100.0.0/org.jabref.gui.journals.AbbreviateAction.lambda$abbreviate$4(AbbreviateAction.java:110)
	at java.base/java.util.stream.MatchOps$1MatchSink.accept(MatchOps.java:90)
	at java.base/java.util.HashMap$KeySpliterator.tryAdvance(HashMap.java:1737)
	at java.base/java.util.stream.ReferencePipeline.forEachWithCancel(ReferencePipeline.java:147)
	at java.base/java.util.stream.AbstractPipeline.copyIntoWithCancel(AbstractPipeline.java:588)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:574)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:560)
	at java.base/java.util.stream.MatchOps$MatchOp.evaluateSequential(MatchOps.java:230)
	at java.base/java.util.stream.MatchOps$MatchOp.evaluateSequential(MatchOps.java:196)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:265)
	at java.base/java.util.stream.ReferencePipeline.anyMatch(ReferencePipeline.java:672)
	at org.jabref@100.0.0/org.jabref.gui.journals.AbbreviateAction.lambda$abbreviate$5(AbbreviateAction.java:109)
	at java.base/java.util.concurrent.FutureTask.run$$$capture(FutureTask.java:317)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
	at java.base/java.lang.Thread.run(Thread.java:1575)

ERROR: Uncaught exception occurred in Thread[#255,JabRef CachedThreadPool,5,main]: java.lang.IndexOutOfBoundsException: Index 0 out of bounds for length 0
	at java.base/java.util.Objects.checkIndex(Objects.java:365)
	at java.base/java.util.ArrayList.get(ArrayList.java:428)
	at javafx.base@23.0.2/javafx.collections.ListChangeBuilder.commit(ListChangeBuilder.java:518)
	at javafx.base@23.0.2/javafx.collections.ListChangeBuilder.endChange(ListChangeBuilder.java:541)
	at javafx.base@23.0.2/javafx.collections.ObservableListBase.endChange(ObservableListBase.java:210)
	at javafx.base@23.0.2/com.sun.javafx.collections.ObservableListWrapper.access$200(ObservableListWrapper.java:46)
	at javafx.base@23.0.2/com.sun.javafx.collections.ObservableListWrapper$1$1.invalidated(ObservableListWrapper.java:74)
	at javafx.base@23.0.2/com.sun.javafx.collections.MapListenerHelper$Generic.fireValueChangedEvent(MapListenerHelper.java:318)
	at javafx.base@23.0.2/com.sun.javafx.collections.MapListenerHelper.fireValueChangedEvent(MapListenerHelper.java:70)
	at javafx.base@23.0.2/com.sun.javafx.collections.ObservableMapWrapper.callObservers(ObservableMapWrapper.java:115)
	at javafx.base@23.0.2/com.sun.javafx.collections.ObservableMapWrapper.put(ObservableMapWrapper.java:169)
	at org.jabref@100.0.0/org.jabref.model.entry.BibEntry.setField(BibEntry.java:640)
	at org.jabref@100.0.0/org.jabref.model.entry.BibEntry.setField(BibEntry.java:658)
	at org.jabref@100.0.0/org.jabref.gui.journals.UndoableAbbreviator.abbreviate(UndoableAbbreviator.java:64)
	at org.jabref@100.0.0/org.jabref.gui.journals.AbbreviateAction.lambda$abbreviate$4(AbbreviateAction.java:110)
	at java.base/java.util.stream.MatchOps$1MatchSink.accept(MatchOps.java:90)
	at java.base/java.util.HashMap$KeySpliterator.tryAdvance(HashMap.java:1737)
	at java.base/java.util.stream.ReferencePipeline.forEachWithCancel(ReferencePipeline.java:147)
	at java.base/java.util.stream.AbstractPipeline.copyIntoWithCancel(AbstractPipeline.java:588)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:574)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:560)
	at java.base/java.util.stream.MatchOps$MatchOp.evaluateSequential(MatchOps.java:230)
	at java.base/java.util.stream.MatchOps$MatchOp.evaluateSequential(MatchOps.java:196)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:265)
	at java.base/java.util.stream.ReferencePipeline.anyMatch(ReferencePipeline.java:672)
	at org.jabref@100.0.0/org.jabref.gui.journals.AbbreviateAction.lambda$abbreviate$5(AbbreviateAction.java:109)
	at java.base/java.util.concurrent.FutureTask.run$$$capture(FutureTask.java:317)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
	at java.base/java.lang.Thread.run(Thread.java:1575)
@subhramit
Copy link
Member

Thanks for opening the issue. The issue description lacks a bit of clarity, so please address the following:

  1. Please refine the steps to reproduce so that it's easier to trigger the bug. What does "Search" refer to in step 2? Do you mean Web Search? Was it a particular fetcher? Or is it reproducible in multiple fetchers? In step 4, do you mean select all imported entries in the library? Is step 5 enough to trigger the errors? Asking because as per our discussion over the Community Chat, you mentioned:

    change the abbreviations to dotless then again to shortUnique.
    If the error doesn't appear, change the abbreviations again.

  2. Please add the full exception logs as generated (either from the IntelliJ terminal or from Help -> View Event log or simply if the full exception pops up as a dialog).

@marwanemad07
Copy link
Contributor Author

2. either from the IntelliJ terminal

The exceptions I put are the same that's in the terminal. I added an investigation of where this error comes from.

@subhramit
Copy link
Member

subhramit commented Mar 7, 2025 •
edited
Loading

The exceptions I put are the same that's in the terminal. I added an investigation of where this error comes from.

That's not a full exception log - that is just the exact line at the end of the stack trace where the exception occurs. Please look up how stack traces look like. For reference, look at the log attached at this issue. If you are unable to grasp, provide a full screenshot of the Intellij run window (terminal area) when the exception occurs.

@marwanemad07
Copy link
Contributor Author

I changed the log file, let me know if I need to edit it again.

@subhramit
Copy link
Member

subhramit commented Mar 7, 2025 •
edited
Loading

I changed the log file, let me know if I need to edit it again.

Nope, it's the right ones now. Thanks again!

@marwanemad07
Copy link
Contributor Author

/assign-me

@github-actions github-actions bot added the 📍 Assigned Assigned by assign-issue-action (or manually assigned) label Mar 10, 2025
@github-actions GitHub Actions
Copy link
Contributor

👋 Hey @marwanemad07, thank you for your interest in this issue! 🎉

We're excited to have you on board. Start by exploring our Contributing guidelines, and don't forget to check out our workspace setup guidelines to get started smoothly.

In case you encounter failing tests during development, please check our developer FAQs!

Having any questions or issues? Feel free to ask here on GitHub. Need help setting up your local workspace? Join the conversation on JabRef's Gitter chat. And don't hesitate to open a (draft) pull request early on to show the direction it is heading towards. This way, you will receive valuable feedback.

Happy coding! 🚀

⏳ Please note, you will be automatically unassigned if the issue isn't closed within 45 days (by 24 April 2025). A maintainer can also add the "📌 Pinned"" label to prevent automatic unassignment.

@marwanemad07
Copy link
Contributor Author

I have a question regarding this issue. In /JabRef/gui/journal/AbbreviateAction.java, why do we use tasks in the abbreviate function but not in the unabbreviate function? I think using tasks makes the function faster; why aren't tasks used in both functions?

@subhramit
Copy link
Member

I have a question regarding this issue. In /JabRef/gui/journal/AbbreviateAction.java, why do we use tasks in the abbreviate function but not in the unabbreviate function? I think using tasks makes the function faster; why aren't tasks used in both functions?

@calixtus

@koppor
Copy link
Member

koppor commented Mar 12, 2025

One could use git blame and then check in which PR the change happened. And then read on. Maybe, there is a reasoning in the discussion, maybe not.

@calixtus
Copy link
Member

Reasoning is not present to me anymore. The PR in what i worked on that was about removing legacy BasePanel and Action from the Codebase to make it more maintainable.
Please mind that platform threads are very expensive on resources to create. Consider moving to Virtual Threads for this action in abbreviate and unabbreviate.

@subhramit
Copy link
Member

subhramit commented Mar 12, 2025 •
edited
Loading

Reasoning is not present to me anymore. The PR in what i worked on that was about removing legacy BasePanel and Action from the Codebase to make it more maintainable. Please mind that platform threads are very expensive on resources to create. Consider moving to Virtual Threads for this action in abbreviate and unabbreviate.

Would that be a good first issue?
Never mind, one can work on that if they work on this issue I guess.

@marwanemad07
Copy link
Contributor Author

marwanemad07 commented Mar 12, 2025 •
edited by subhramit
Loading

Reasoning is not present to me anymore. The PR in what i worked on that was about removing legacy BasePanel and Action from the Codebase to make it more maintainable.

I found the first issue related to the performance in the abbreviate function was #2831 and in this PR #3721 introduced a first solution for using threads.

In this PR #6056 you introduced the unabbreviate function, I think it was intended not to use threads in it since you updated in the abbreviate function.

@marwanemad07 marwanemad07 linked a pull request Mar 12, 2025 that will close this issue
Open
7 tasks
@github-actions github-actions bot removed the 📍 Assigned Assigned by assign-issue-action (or manually assigned) label Mar 12, 2025
@koppor
Copy link
Member

koppor commented Mar 12, 2025

In this PR #6056 you introduced the unabbreviate function, I think it was intended not to use threads in it since you updated in the abbreviate function.

The link is nice, but the unabbreviate is removed (and integrated into Abbreviate..)

https://github.com/JabRef/jabref/pull/6056/files#diff-ed1d9e2286c158bf3044edb62b2fa31027d3b41b4f42fb23c9ea42cfbec9f4ea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
component: journal abbreviations good third issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix threading issues in AbbreviateAction class marwanemad07/jabref
4 participants
@koppor @calixtus @subhramit @marwanemad07