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

Fixes issue where Unix crashes when inotify is full #6637

Merged
merged 7 commits into from Jul 1, 2020
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
- We fixed an issue where downloaded files would be moved to a directory named after the citationkey when no file directory pattern is specified [#6589](https://github.com/JabRef/jabref/issues/6589)
- We fixed an issue with the creation of a group of cited entries which incorrectly showed the message that the library had been modified externally whenever saving the library. [#6420](https://github.com/JabRef/jabref/issues/6420)
- We fixed an issue with the creation of a group of cited entries. Now the file path to an aux file gets validated. [#6585](https://github.com/JabRef/jabref/issues/6585)
- We fixed an issue on Linux systems where the application would crash upon inotify failure. Now, the user is prompted with a warning, and given the choice to continue the session. [#6073](https://github.com/JabRef/jabref/issues/6073)

### Removed

Expand Down
7 changes: 7 additions & 0 deletions src/main/java/org/jabref/JabRefGUI.java
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,13 @@ private void openWindow(Stage mainStage) {
}
});
Platform.runLater(this::openDatabases);

if (!(Globals.getFileUpdateMonitor().isActive())) {
this.getMainFrame().getDialogService()
.showErrorDialogAndWait(Localization.lang("Unable to monitor file changes. Please close files " +
This conversation was marked as resolved.
Show resolved Hide resolved
"and processes and restart. You may encounter errors if you continue " +
"with this session."));
}
}

private void openDatabases() {
Expand Down
29 changes: 20 additions & 9 deletions src/main/java/org/jabref/gui/util/DefaultFileUpdateMonitor.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@
import java.nio.file.WatchEvent;
import java.nio.file.WatchKey;
import java.nio.file.WatchService;
import java.util.Objects;
import java.util.Optional;

import org.jabref.JabRefException;
import org.jabref.model.util.FileUpdateListener;
import org.jabref.model.util.FileUpdateMonitor;
import org.jabref.model.util.WatchServiceUnavailableException;

import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.Multimap;
Expand All @@ -29,11 +31,14 @@ public class DefaultFileUpdateMonitor implements Runnable, FileUpdateMonitor {

private final Multimap<Path, FileUpdateListener> listeners = ArrayListMultimap.create(20, 4);
private WatchService watcher;
private Optional<JabRefException> filesystemMonitorFailure;
Copy link
Member

Choose a reason for hiding this comment

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

I think, it's OK to store the exception (for future work).

Copy link
Author

Choose a reason for hiding this comment

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

What do you mean? I am not sure what the code change is that you are requesting.

Copy link
Member

Choose a reason for hiding this comment

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

This was more a comment to a second reviewer.

Copy link
Author

Choose a reason for hiding this comment

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


@Override
public void run() {
try (WatchService watcher = FileSystems.getDefault().newWatchService()) {
this.watcher = watcher;
filesystemMonitorFailure = Optional.empty();

while (true) {
WatchKey key;
try {
Expand All @@ -59,23 +64,29 @@ public void run() {
}
Thread.yield();
}
} catch (Throwable e) {
LOGGER.error("FileUpdateMonitor has been interrupted.", e);
} catch (IOException e) {
filesystemMonitorFailure = Optional.of(new WatchServiceUnavailableException(e.getMessage(),
e.getLocalizedMessage(), e.getCause()));
LOGGER.warn(filesystemMonitorFailure.get().getLocalizedMessage(), e);
}
}

public boolean isActive() {
return filesystemMonitorFailure.isEmpty();
}

private void notifyAboutChange(Path path) {
listeners.get(path).forEach(FileUpdateListener::fileUpdated);
}

@Override
public void addListenerForFile(Path file, FileUpdateListener listener) throws IOException {
Objects.requireNonNull(watcher, "You need to start the file monitor before watching files");

// We can't watch files directly, so monitor their parent directory for updates
Path directory = file.toAbsolutePath().getParent();
directory.register(watcher, StandardWatchEventKinds.ENTRY_MODIFY);
listeners.put(file, listener);
if (isActive()) {
// We can't watch files directly, so monitor their parent directory for updates
Path directory = file.toAbsolutePath().getParent();
directory.register(watcher, StandardWatchEventKinds.ENTRY_MODIFY);
listeners.put(file, listener);
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package org.jabref.model.util;

import java.io.IOException;
import java.nio.file.Path;

/**
Expand All @@ -9,12 +8,17 @@
*/
public class DummyFileUpdateMonitor implements FileUpdateMonitor {
@Override
public void addListenerForFile(Path file, FileUpdateListener listener) throws IOException {
public void addListenerForFile(Path file, FileUpdateListener listener) {
Copy link
Member

Choose a reason for hiding this comment

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

The exception was removed here, but not in the interface. I think, it should also be removed here (and the JavaDoc be adapted).

With the current implementation, it does not throw an exception if the file does not exist.


}

@Override
public void removeListener(Path path, FileUpdateListener listener) {

}

@Override
public boolean isActive() {
return false;
}
}
6 changes: 6 additions & 0 deletions src/main/java/org/jabref/model/util/FileUpdateMonitor.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,10 @@ public interface FileUpdateMonitor {
* @param path The path to remove.
*/
void removeListener(Path path, FileUpdateListener listener);

/**
* Indicates whether or not the native system's file monitor has successfully started.
* @return True is process is running; false otherwise.
*/
boolean isActive();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package org.jabref.model.util;

import org.jabref.JabRefException;

public class WatchServiceUnavailableException extends JabRefException {
public WatchServiceUnavailableException(final String message, final String localizedMessage, final Throwable cause) {
super(message, localizedMessage, cause);
}
}
1 change: 1 addition & 0 deletions src/main/resources/l10n/JabRef_en.properties
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
Unable\ to\ monitor\ file\ changes.\ Please\ close\ files\ and\ processes\ and\ restart.\ You\ may\ encounter\ errors\ if\ you\ continue\ with\ this\ session.=Unable to monitor file changes. Please close files and processes and restart. You may encounter errors if you continue with this session.
%0\ contains\ the\ regular\ expression\ <b>%1</b>=%0 contains the regular expression <b>%1</b>

%0\ contains\ the\ term\ <b>%1</b>=%0 contains the term <b>%1</b>
Expand Down