Skip to content

Fix exception when searching#7659

Merged
Siedlerchr merged 5 commits into
JabRef:mainfrom
Fifi-wang:work
Apr 24, 2021
Merged

Fix exception when searching#7659
Siedlerchr merged 5 commits into
JabRef:mainfrom
Fifi-wang:work

Conversation

@Fifi-wang

@Fifi-wang Fifi-wang commented Apr 21, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

We fix the issue that uncaught exception when searching in dark mode. In fact, we found that this issue is not a dark mode issue and this is a threading issue. So we use Platform.runLater() to ensure that there will be no conflicts when adding key-value pairs to the map.
fixes #7343

  • 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 documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

@Siedlerchr Siedlerchr changed the title Fix for issue 7343 Fix exceotion when searching Apr 21, 2021
@Siedlerchr Siedlerchr changed the title Fix exceotion when searching Fix exception when searching Apr 21, 2021
@Siedlerchr Siedlerchr requested a review from tobiasdiez April 21, 2021 17:12
@tobiasdiez

tobiasdiez commented Apr 22, 2021

Copy link
Copy Markdown
Member

Thanks a lot for the investigation and the fix!

However, I fear that the proposed changes will lead to other threading problems because currently setField is used with the assumption that changes are immediately stored in the entry. This is no longer the case when using Platform.runLater. Thus, I would propose to fix this at a later stage in the call hierarchy, e.g. in the import dialog. Do you know which code exactly leads to the threading issue?

@Fifi-wang

Fifi-wang commented Apr 23, 2021

Copy link
Copy Markdown
Contributor Author

Thanks a lot for the investigation and the fix!

However, I fear that the proposed changes will lead to other threading problems because currently setField is used with the assumption that changes are immediately stored in the entry. This is no longer the case when using Platform.runLater. Thus, I would propose to fix this at a later stage in the call hierarchy, e.g. in the import dialog. Do you know which code exactly leads to the threading issue?

I apologize for not considering other thread issues. The threading problem here mainly occurs in the put and remove methods I modified. Because the ObservableMap used here is not a thread-safe mapping. Therefore, there will be conflicts between put and remove.

@tobiasdiez

tobiasdiez commented Apr 23, 2021

Copy link
Copy Markdown
Member

No worries!

Do you know which code is invoking put and remove from different threads?

@Fifi-wang

Fifi-wang commented Apr 23, 2021

Copy link
Copy Markdown
Contributor Author

org.jabref.logic.importer.ImportCleanup#doPostCleanup(java.util.Collection<org.jabref.model.entry.BibEntry>)
This function use ParallelStream to invoke doPostCleanup function where make thread conflicts.
org.jabref.model.entry.BibEntry#clearField(org.jabref.model.entry.field.Field, org.jabref.model.entry.event.EntriesEventSource)
org.jabref.model.entry.BibEntry#setField(org.jabref.model.entry.field.Field, java.lang.String, org.jabref.model.entry.event.EntriesEventSource)
These two functions will invoke put and remove directly.

@tobiasdiez

tobiasdiez commented Apr 23, 2021

Copy link
Copy Markdown
Member

If the parallel stream is the source for the problems, then just use a normal stream.

tobiasdiez
tobiasdiez approved these changes Apr 24, 2021
View reviewed changes

@tobiasdiez tobiasdiez left a comment

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.

Thanks for the quick follow-up!

Siedlerchr
Siedlerchr reviewed Apr 24, 2021
View reviewed changes
Comment thread CHANGELOG.md Outdated
@Siedlerchr Siedlerchr merged commit 4bf895b into JabRef:main Apr 24, 2021
Siedlerchr added a commit that referenced this pull request Apr 24, 2021
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into dependabot/gradle/c…
d87fb61 
…om.tngtech.archunit-archunit-junit5-api-0.18.0

* upstream/main:
  Fix exception when searching (#7659)
  Fixes Jabref#7660 (#7663)
  Fix for issue 5850: Journal abbreviations in UTF-8 not recognized (#7639)
  Fix SSLHandshake Exception by using bypass (#7657)
  Fix for issue 7633: Unable to download arXiv pdfs if Title contains curly brackets (#7652)
  Fix#7195 partly Opacity of disabled icon-buttons
Siedlerchr added a commit that referenced this pull request Apr 26, 2021
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into gradlew-update-7.0
76a4715 
* upstream/main:
  Bump checkstyle from 8.41.1 to 8.42 (#7668)
  Bump postgresql from 42.2.19 to 42.2.20 (#7670)
  Bump org.beryx.jlink from 2.23.6 to 2.23.7 (#7669)
  Fix exception when searching (#7659)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Siedlerchr Siedlerchr Siedlerchr approved these changes

@tobiasdiez tobiasdiez tobiasdiez approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uncaught exception when searching in dark mode

4 participants

@Fifi-wang @tobiasdiez @Siedlerchr @Rico0727