Fix freezing when running cleanup file operations like rename #3315

Siedlerchr · 2017-10-17T19:52:13Z

I noticed a problem when executing a rename cleanup operation. It lead to a freeze.
The only thing is that currently the DefaultTaskExectutor resides in gui. Moving it to logic or model would be possible (but still not the best idea)

Change in CHANGELOG.md described
Tests created for changes
Screenshots added (for bigger UI changes)
Manually tested changed features in running JabRef
Check documentation status (Issue created for outdated help page at help.jabref.org?)
If you changed the localization: Did you run gradle localizationUpdate?

LinusDietz · 2017-10-17T22:49:27Z

src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java

+            Platform.runLater(task);
+            try {
+                return task.get();
+            } catch (InterruptedException | ExecutionException e) {


You need a guarded wait here in case an InterruptedException is thrown

I am not sure what you mean: The get already waits for completion. the only thing which we could add is a timeout. But that depends

LinusDietz · 2017-10-17T22:52:11Z

src/main/java/org/jabref/model/entry/BibEntry.java

@@ -804,7 +805,8 @@ private void invalidateFieldCache(String fieldName) {
            return Optional.empty();
        }

-        return this.setField(FieldName.FILE, newValue);
+        //Violates our architecture, but currently only possible way because the LinkedFile is setup using JavaFX properties
+        return DefaultTaskExecutor.runInJavaFXThread(() -> this.setField(FieldName.FILE, newValue));


that's a problem of our model then. I would vote to move the setup of the linked file to the model instead of violating the architecture in this way.

There shouldn't be any problem with setting a JavaFX property on a non-FX thread. The properties don't check on which thread they are invoked (e.g. I think the basic example by oracle about the binding stuff actually uses a command line application). The problem occurs if you bind a JavaFX control to these properties, since then you get a cascade like
set javafx property > set some property in the control via binding > update view
and the last step is expected to be on the fx thread.

So far for the general case. Now here, where do we use bindings on the file field?

In fact, updateItem on the LIstviewCellFactory is called which in the end leads to the thread error.

1:46:47.408 [Spin-2] ERROR org.jabref.FallbackExceptionHandler - Uncaught exception occurred in Thread[Spin-2,6,spin] java.lang.IllegalStateException: Not on FX application thread; currentThread = Spin-2 at com.sun.javafx.tk.Toolkit.checkFxUserThread(Toolkit.java:236) ~[jfxrt.jar:?] at com.sun.javafx.tk.quantum.QuantumToolkit.checkFxUserThread(QuantumToolkit.java:423) ~[jfxrt.jar:?] at javafx.scene.Parent$2.onProposedChange(Parent.java:367) ~[jfxrt.jar:?] at com.sun.javafx.collections.VetoableListDecorator.setAll(VetoableListDecorator.java:113) ~[jfxrt.jar:?] at com.sun.javafx.collections.VetoableListDecorator.setAll(VetoableListDecorator.java:108) ~[jfxrt.jar:?] at com.sun.javafx.scene.control.skin.LabeledSkinBase.updateChildren(LabeledSkinBase.java:575) ~[jfxrt.jar:?] at com.sun.javafx.scene.control.skin.LabeledSkinBase.handleControlPropertyChanged(LabeledSkinBase.java:181) ~[jfxrt.jar:?] at com.sun.javafx.scene.control.skin.ListCellSkin.handleControlPropertyChanged(ListCellSkin.java:49) ~[jfxrt.jar:?] at com.sun.javafx.scene.control.skin.BehaviorSkinBase.lambda$registerChangeListener$61(BehaviorSkinBase.java:197) ~[jfxrt.jar:?] at com.sun.javafx.scene.control.MultiplePropertyChangeListenerHandler$1.changed(MultiplePropertyChangeListenerHandler.java:55) ~[jfxrt.jar:?] at javafx.beans.value.WeakChangeListener.changed(WeakChangeListener.java:89) ~[jfxrt.jar:?] at com.sun.javafx.binding.ExpressionHelper$SingleChange.fireValueChangedEvent(ExpressionHelper.java:182) ~[jfxrt.jar:?] at com.sun.javafx.binding.ExpressionHelper.fireValueChangedEvent(ExpressionHelper.java:81) ~[jfxrt.jar:?] at javafx.beans.property.ObjectPropertyBase.fireValueChangedEvent(ObjectPropertyBase.java:105) ~[jfxrt.jar:?] at javafx.beans.property.ObjectPropertyBase.markInvalid(ObjectPropertyBase.java:112) ~[jfxrt.jar:?] at javafx.beans.property.ObjectPropertyBase.set(ObjectPropertyBase.java:146) ~[jfxrt.jar:?] at javafx.css.StyleableObjectProperty.set(StyleableObjectProperty.java:82) ~[jfxrt.jar:?] at javafx.beans.property.ObjectProperty.setValue(ObjectProperty.java:69) ~[jfxrt.jar:?] at javafx.scene.control.Labeled.setGraphic(Labeled.java:423) ~[jfxrt.jar:?] at org.jabref.gui.util.ViewModelListCellFactory$1.updateItem(ViewModelListCellFactory.java:122) ~[bin/:?] at javafx.scene.control.ListCell.updateItem(ListCell.java:480) ~[jfxrt.jar:?] at javafx.scene.control.ListCell.lambda$new$160(ListCell.java:167) ~[jfxrt.jar:?] at javafx.collections.WeakListChangeListener.onChanged(WeakListChangeListener.java:88) ~[jfxrt.jar:?] at com.sun.javafx.collections.ListListenerHelper$Generic.fireValueChangedEvent(ListListenerHelper.java:329) ~[jfxrt.jar:?] at com.sun.javafx.collections.ListListenerHelper.fireValueChangedEvent(ListListenerHelper.java:73) ~[jfxrt.jar:?] at javafx.collections.ObservableListBase.fireChange(ObservableListBase.java:233) ~[jfxrt.jar:?] at javafx.collections.ListChangeBuilder.commit(ListChangeBuilder.java:482) ~[jfxrt.jar:?] at javafx.collections.ListChangeBuilder.endChange(ListChangeBuilder.java:541) ~[jfxrt.jar:?] at javafx.collections.ObservableListBase.endChange(ObservableListBase.java:205) ~[jfxrt.jar:?] at com.sun.javafx.collections.ObservableListWrapper.remove(ObservableListWrapper.java:167) ~[jfxrt.jar:?] at com.sun.javafx.binding.BidirectionalContentBinding$ListContentBinding.onChanged(BidirectionalContentBinding.java:135) ~[jfxrt.jar:?] at com.sun.javafx.binding.ListExpressionHelper$Generic.notifyListeners(ListExpressionHelper.java:593) ~[jfxrt.jar:?] at com.sun.javafx.binding.ListExpressionHelper$Generic.fireValueChangedEvent(ListExpressionHelper.java:571) ~[jfxrt.jar:?] at com.sun.javafx.binding.ListExpressionHelper.fireValueChangedEvent(ListExpressionHelper.java:109) ~[jfxrt.jar:?] at javafx.beans.property.ListPropertyBase.fireValueChangedEvent(ListPropertyBase.java:200) ~[jfxrt.jar:?] at javafx.beans.property.ListPropertyBase.lambda$new$39(ListPropertyBase.java:56) ~[jfxrt.jar:?] at com.sun.javafx.collections.ListListenerHelper$SingleChange.fireValueChangedEvent(ListListenerHelper.java:164) ~[jfxrt.jar:?] at com.sun.javafx.collections.ListListenerHelper.fireValueChangedEvent(ListListenerHelper.java:73) ~[jfxrt.jar:?] at javafx.collections.ObservableListBase.fireChange(ObservableListBase.java:233) ~[jfxrt.jar:?] at javafx.collections.ListChangeBuilder.commit(ListChangeBuilder.java:482) ~[jfxrt.jar:?] at javafx.collections.ListChangeBuilder.endChange(ListChangeBuilder.java:541) ~[jfxrt.jar:?] at javafx.collections.ObservableListBase.endChange(ObservableListBase.java:205) ~[jfxrt.jar:?] at javafx.collections.ModifiableObservableListBase.setAll(ModifiableObservableListBase.java:90) ~[jfxrt.jar:?] at javafx.beans.binding.ListExpression.setAll(ListExpression.java:370) ~[jfxrt.jar:?] at org.jabref.gui.util.BindingsHelper.lambda$5(BindingsHelper.java:121) ~[bin/:?] at org.jabref.gui.util.BindingsHelper$BidirectionalListBinding.changed(BindingsHelper.java:192) ~[bin/:?] at com.sun.javafx.binding.ExpressionHelper$Generic.fireValueChangedEvent(ExpressionHelper.java:361) ~[jfxrt.jar:?] at com.sun.javafx.binding.ExpressionHelper.fireValueChangedEvent(ExpressionHelper.java:81) ~[jfxrt.jar:?] at javafx.beans.property.StringPropertyBase.fireValueChangedEvent(StringPropertyBase.java:103) ~[jfxrt.jar:?] at javafx.beans.property.StringPropertyBase.markInvalid(StringPropertyBase.java:110) ~[jfxrt.jar:?] at javafx.beans.property.StringPropertyBase.set(StringPropertyBase.java:144) ~[jfxrt.jar:?] at javafx.beans.property.StringPropertyBase.set(StringPropertyBase.java:49) ~[jfxrt.jar:?] at javafx.beans.property.StringProperty.setValue(StringProperty.java:65) ~[jfxrt.jar:?] at javafx.beans.property.StringProperty.setValue(StringProperty.java:57) ~[jfxrt.jar:?] at org.jabref.gui.util.BindingsHelper$BidirectionalBinding.updateLocked(BindingsHelper.java:164) ~[bin/:?] at org.jabref.gui.util.BindingsHelper$BidirectionalBinding.changedB(BindingsHelper.java:157) ~[bin/:?] at com.sun.javafx.binding.ExpressionHelper$SingleChange.fireValueChangedEvent(ExpressionHelper.java:182) [jfxrt.jar:?] at com.sun.javafx.binding.ExpressionHelper.fireValueChangedEvent(ExpressionHelper.java:81) [jfxrt.jar:?] at javafx.beans.binding.ObjectBinding.invalidate(ObjectBinding.java:172) [jfxrt.jar:?] at com.sun.javafx.binding.BindingHelperObserver.invalidated(BindingHelperObserver.java:51) [jfxrt.jar:?] at com.sun.javafx.collections.MapListenerHelper$Generic.fireValueChangedEvent(MapListenerHelper.java:320) [jfxrt.jar:?] at com.sun.javafx.collections.MapListenerHelper.fireValueChangedEvent(MapListenerHelper.java:72) [jfxrt.jar:?] at com.sun.javafx.collections.ObservableMapWrapper.callObservers(ObservableMapWrapper.java:115) [jfxrt.jar:?] at com.sun.javafx.collections.ObservableMapWrapper.put(ObservableMapWrapper.java:169) [jfxrt.jar:?] at org.jabref.model.entry.BibEntry.setField(BibEntry.java:422) [bin/:?] at org.jabref.model.entry.BibEntry.setField(BibEntry.java:448) [bin/:?] at org.jabref.model.entry.BibEntry.setFiles(BibEntry.java:807) [bin/:?]

I think the best solution is actually to call the whole cleanup process in the JavaFX thread. If this does not work, you can add the Platform.runLater also in org.jabref.gui.util.BindingsHelper

It works in that case that I do not receive any errors, but the file isn't renamed. I think the problem is that somehow it immediately returns instead of waiting

tobiasdiez · 2017-10-18T01:46:15Z

src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java

+        if (!Platform.isFxApplicationThread()) {
+            Platform.runLater(runnable);
+        } else {
+            LOGGER.debug("Already on FX thread");


You should make sure to also invoke the runnable here at all times!

tobiasdiez · 2017-10-18T01:47:39Z

src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java

+        } else {
+            try {
+                //we directly execute the callback
+                return callable.call();


runLater is async, while this code is sync. I don't like that we change behavior depending on whether we are in a javafx thread or not. What was your idea behind those changes?

I did not really think about it. will wrap it.

tobiasdiez · 2017-10-18T01:59:17Z

src/main/java/org/jabref/model/entry/BibEntry.java

@@ -804,7 +805,8 @@ private void invalidateFieldCache(String fieldName) {
            return Optional.empty();
        }

-        return this.setField(FieldName.FILE, newValue);
+        //Violates our architecture, but currently only possible way because the LinkedFile is setup using JavaFX properties
+        return DefaultTaskExecutor.runInJavaFXThread(() -> this.setField(FieldName.FILE, newValue));


There shouldn't be any problem with setting a JavaFX property on a non-FX thread. The properties don't check on which thread they are invoked (e.g. I think the basic example by oracle about the binding stuff actually uses a command line application). The problem occurs if you bind a JavaFX control to these properties, since then you get a cascade like
set javafx property > set some property in the control via binding > update view
and the last step is expected to be on the fx thread.

So far for the general case. Now here, where do we use bindings on the file field?

Update call of submit

lenhard

I guess you were asking for my review because of the architectural considerations? Well, I would be very much in favor of not adding the executor into model. You have solved that now by executing the complete cleanup operations on the FX thread and that is fine by me.

The rest of the code looks fine to me as well. Checkstyle just wants you to capitalize the executor variable.

I have tested the PR locally and renaming works as expected without blocking. So please fix the checkstyle issue. If there is an issue to be referenced, then please add a changelog entry.

When these small changes are made, the PR is good to go.

* upstream/master: (31 commits) Source tab entry duplication (#3360) Use CITE_COMMENT not only for external latex editors but also for cop… (#3351) Updating with new translations (#3348) Upgrade error-prone (#3350) Jabref_pt_BR partially updated (#3349) Used late initialization for context menus (#3340) Fix NPE when calling with bib file as cmd argument (#3343) update mockito-core from 2.10.0 -> 2.11.0 (#3338) Remove underscore in Localized messages (#3336) Localisation: French: new entries translated (#3337) Refactoring: Lazy init of all editor tabs (#3333) Initializing EntryEditor Tabs on focus (#3331) Fix #3292: annotations are now automatically refreshed (#3325) Change integrity message for names depending on database mode (#3330) Fix location bundle with fast access (#3327) Small code cleanup Fix "path not found" exception Small fix in drag and drop handler of linked files (#3328) Fix NPE in MainTable (#3318) Increase relative size of abstract field in editor (#3320) ...

* upstream/master: Rework AutoSetFileLinks (#3368) revert wrongly commited changes Refactorings (#3370) Fix freezing when running cleanup file operations like rename (#3315) This additional style setting for IDEA (#3355) Fix checkstyle Add proper equals to content selector Source tab entry duplication (#3360) Use CITE_COMMENT not only for external latex editors but also for cop… (#3351) Updating with new translations (#3348) Upgrade error-prone (#3350) Jabref_pt_BR partially updated (#3349) Delete unused code Extract difference finder from gui to logic Used late initialization for context menus (#3340) Remove unnecessary EntrySorter Move existing code to gui and rename change classes to view model Small code improvements

* upstream/master: (79 commits) Update richtext and flowless (#3386) Source tab: hide notification pane when code is correct again (#3387) Titles are optional at crossref (#3378) Update entry for every change in the source panel (#3366) Rework AutoSetFileLinks (#3368) revert wrongly commited changes Refactorings (#3370) Fix freezing when running cleanup file operations like rename (#3315) This additional style setting for IDEA (#3355) Fix checkstyle Add proper equals to content selector Source tab entry duplication (#3360) Use CITE_COMMENT not only for external latex editors but also for cop… (#3351) Updating with new translations (#3348) Upgrade error-prone (#3350) Jabref_pt_BR partially updated (#3349) Delete unused code Extract difference finder from gui to logic Used late initialization for context menus (#3340) Fix NPE when calling with bib file as cmd argument (#3343) ...

* upstream/master: (22 commits) Improve SyncLang.py (#3184) Config intellj code style to adhere to empty lines checkstyle rule (#3365) Don't list same look and feels more than once (#3393) progessdialog and result table are now shown correclty on copy files Export pdf/linked files (#3147) Added checking integrity dialog (#3388) Update richtext and flowless (#3386) Source tab: hide notification pane when code is correct again (#3387) Titles are optional at crossref (#3378) Update entry for every change in the source panel (#3366) Rework AutoSetFileLinks (#3368) revert wrongly commited changes Refactorings (#3370) Fix freezing when running cleanup file operations like rename (#3315) This additional style setting for IDEA (#3355) Fix checkstyle Add proper equals to content selector Delete unused code Extract difference finder from gui to logic Remove unnecessary EntrySorter ... # Conflicts: # src/main/java/org/jabref/gui/entryeditor/SourceTab.java

* upstream/master: (26 commits) Fix static localized text (#3400) Fix problems in source editor (#3398) Fix MathSciNet fetcher (#3402) Fix NPE when saving new file Improve SyncLang.py (#3184) Config intellj code style to adhere to empty lines checkstyle rule (#3365) Don't list same look and feels more than once (#3393) progessdialog and result table are now shown correclty on copy files Export pdf/linked files (#3147) Added checking integrity dialog (#3388) Update richtext and flowless (#3386) Source tab: hide notification pane when code is correct again (#3387) Titles are optional at crossref (#3378) Update entry for every change in the source panel (#3366) Rework AutoSetFileLinks (#3368) revert wrongly commited changes Refactorings (#3370) Fix freezing when running cleanup file operations like rename (#3315) This additional style setting for IDEA (#3355) Fix checkstyle ... # Conflicts: # CHANGELOG.md

Fix freezing when running cleanup file operations like rename

ce4c690

Siedlerchr requested review from tobiasdiez and lenhard October 17, 2017 19:52

LinusDietz requested changes Oct 17, 2017

View reviewed changes

tobiasdiez reviewed Oct 18, 2017

View reviewed changes

Siedlerchr added 2 commits October 18, 2017 12:08

Remove DefaultTaskExecutor from setField

61e7d11

Update call of submit

add default task executor to cleanup ops

d1cf1b6

lenhard approved these changes Oct 27, 2017

View reviewed changes

Siedlerchr added 2 commits October 27, 2017 18:09

fix checkstyle

1f0dad4

Siedlerchr merged commit 2de829e into master Oct 28, 2017

Siedlerchr deleted the fixSetFiles branch October 28, 2017 18:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix freezing when running cleanup file operations like rename #3315

Fix freezing when running cleanup file operations like rename #3315

Siedlerchr commented Oct 17, 2017 •

edited

Loading

LinusDietz Oct 17, 2017

Siedlerchr Oct 18, 2017

LinusDietz Oct 17, 2017

tobiasdiez Oct 18, 2017

Siedlerchr Oct 18, 2017

tobiasdiez Oct 18, 2017

Siedlerchr Oct 18, 2017

tobiasdiez Oct 18, 2017

tobiasdiez Oct 18, 2017

Siedlerchr Oct 18, 2017

tobiasdiez Oct 18, 2017

lenhard left a comment

Fix freezing when running cleanup file operations like rename #3315

Fix freezing when running cleanup file operations like rename #3315

Conversation

Siedlerchr commented Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lenhard left a comment

Choose a reason for hiding this comment

Siedlerchr commented Oct 17, 2017 •

edited

Loading