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

Fix exception when searching #7659

Merged
merged 5 commits into from
Apr 24, 2021
Merged

Fix exception when searching #7659

merged 5 commits into from
Apr 24, 2021

Conversation

Fifi-wang
Copy link
Contributor

@Fifi-wang Fifi-wang commented Apr 21, 2021

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
@tobiasdiez
Copy link
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
Copy link
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
Copy link
Member

No worries!

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

@Fifi-wang
Copy link
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
Copy link
Member

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

Copy link
Member

@tobiasdiez tobiasdiez left a comment

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!

CHANGELOG.md Outdated Show resolved Hide resolved
@Siedlerchr Siedlerchr merged commit 4bf895b into JabRef:main Apr 24, 2021
Siedlerchr added a commit that referenced this pull request Apr 24, 2021
…om.tngtech.archunit-archunit-junit5-api-0.18.0

* upstream/main:
  Fix exception when searching (#7659)
  Fixes #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
* 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uncaught exception when searching in dark mode
3 participants