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

Increase the network connection timeout and improve error message #8894

Merged
merged 1 commit into from
Jun 12, 2022
Merged

Increase the network connection timeout and improve error message #8894

merged 1 commit into from
Jun 12, 2022

Conversation

jakkarth
Copy link
Contributor

In some instances, connecting to an existing JabRef instance may take longer
than the original 200ms timeout. Here we increase that timeout to a full
second. If it takes longer than that to connect to an existing instance,
chances are something else bad is going on.

Additionally, in a case where we attempt a ping but it fails for some reason,
but we're also unable to bind the port to listen ourselves, we give the
user a more informative error message about the potential cause of that
problem, and offer two possible solutions: figure out what other process
is already binding the port, or file a bug if that process happens to be
JabRef (since that would indicate that a 1s timeout isn't sufficient).

Fixes #8653.

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

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jun 10, 2022
Copy link

@Siphalor Siphalor left a comment

Choose a reason for hiding this comment

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

Looks good - well it isn't that much ^^

As mentioned below, I'd provide a bit more details in the changelog (with a reference to the issue).

CHANGELOG.md Outdated Show resolved Hide resolved
@jakkarth jakkarth marked this pull request as ready for review June 10, 2022 22:27
Comment on lines 46 to 48
LOGGER.warn("There was an error opening the configured network port {}. Please ensure there isn't another" +
" application already using that port. If you're sure an existing JabRef instance is running and" +
" using this port, and you see this message, please file a bug.", port);
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for your effort in improving JabRef user experience. Changes look good, except one small nitpick:
The message seems to be a bit too detailed. We could put the third sentence below almost every logger message...
Be aware that the Logger already provides extensive information about the class where the warning has risen etc.
I believe the first two sentences are sufficient about the port in use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I appreciate the feedback! I wanted the message to try to cover both potential cases:

  1. Some other application is already using that port, so we can't bind it
  2. JabRef is already using that port, and the ping timed out even after increasing to 1s

If we remove the third sentence, we lose point number 2. I encountered this issue and the only clue I got was that JabRef couldn't bind the port, nothing about the ping failing. Without the third sentence, a future user who hits the timeout issue will face the same lack of clues that I did. Do you have a suggestion for how we could phrase the message to cover both points? Or do you think that, post change, this scenario is unlikely enough to be encountered that it's better to shorten the message at the expense of that information? I'm happy to remove it if you think that's the better course.

Copy link
Member

Choose a reason for hiding this comment

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

Sentences are:

  1. There was an error opening the configured network port {}.
  2. Please ensure there isn't another application already using that port.
  3. If you're sure an existing JabRef instance is running and using this port, and you see this message, please file a bug.

The meaning of the first part of sentence three is implicitly covered by the second. I expect the interested user who actually reads the log files to be clever enough to reach out to our support resources. ;-)

Please just remove sentence three...

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 we should put that in the documentation for troubleshooting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

Copy link
Member

Choose a reason for hiding this comment

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

Where in the documentation can I find it? I checked https://github.com/JabRef/user-documentation/pulls, however I don't find an open (or closed) PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what we would put in the documentation, since the timeout is not configurable. I'm happy to make a PR for the documentation project, I'm just not sure what you would like me to add there?

In some instances, connecting to an existing JabRef instance may take longer
than the original 200ms timeout. Here we increase that timeout to a full
second. If it takes longer than that to connect to an existing instance,
chances are something else bad is going on.

Additionally, in a case where we attempt a ping but it fails for some reason,
but we're also unable to bind the port to listen ourselves, we give the
user a more informative error message about the potential cause of that
problem, and offer two possible solutions: figure out what other process
is already binding the port, or file a bug if that process happens to be
JabRef (since that would indicate that a 1s timeout isn't sufficient).
@Siedlerchr Siedlerchr merged commit e3b2ab2 into JabRef:main Jun 12, 2022
@Siedlerchr
Copy link
Member

Thank you very much for your first contribution @jakkarth 🥇 Hope you were not deterred by the "nitpicking" ;) We just want to make sure that messages are understandable for the end user.

Siedlerchr added a commit that referenced this pull request Jun 23, 2022
* upstream/main: (27 commits)
  Add gitpod config (#8921)
  Fix javafx version not displayed in About Dialog (#8918)
  Redesign "Manage field names & content" dialog (#8892)
  Rework IACR fetcher (#8904)
  Bump h2-mvstore from 2.1.212 to 2.1.214 in /buildSrc (#8915)
  Bump unoloader from 7.3.3 to 7.3.4 (#8912)
  Bump libreoffice from 7.3.3 to 7.3.4 (#8913)
  Bump tika-core from 2.4.0 to 2.4.1 (#8914)
  Bump org.eclipse.jgit from 6.1.0.202203080745-r to 6.2.0.202206071550-r (#8916)
  Squashed 'buildres/csl/csl-styles/' changes from e740261..845dee0 (#8903)
  Bump flowless from 0.6.9 to 0.6.10 (#8898)
  Bump postgresql from 42.3.5 to 42.4.0 (#8900)
  Bump mockito-core from 4.6.0 to 4.6.1 (#8899)
  Bump antlr-runtime from 3.5.2 to 3.5.3 (#8897)
  Update to MADR 3.0.0-beta.2 (#8896)
  Increase the network connection timeout and improve error message (#8894)
  Fix linux terminal opening process error (#8891)
  Fix exception if linked file has masked umlauts / invalid characters in path (#8851)
  Remove obsolte CHANGELOG.md entry
  Revert "Fix right clicking a group and choosing "remove selected entries from this group" leads to error when Bibtex source tab is selected (#8821)"
  ...

# Conflicts:
#	build.gradle
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JabRef opens multiple instances even when Remote operation is enabled
6 participants