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

Add hints to port conflict and lock file panics #1535

Merged
merged 61 commits into from
Jan 29, 2021

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Dec 17, 2020

Motivation

The port conflict and the lock file messages are hard to understand. We want to add a hint so the user can rerun with a fixed configuration.

Solution

We add 2 panics in this PR with custom messages. We filter the issue url as we don't want users to report this as it is expected to happen if they have another instance of zebrad running in the same machine.

There are 3 different ports that can conflict, and the lock file can conflict:

We need to cover all 4 cases in this PR.

The code in this pull request has:

  • Documentation Comments
  • Unit Tests and Property Tests

Review

Maybe @yaahc and/or @teor2345 . Not urgent.

Related Issues

Close #1484, close #1508, close #1632, close #1509 if merged.

Follow-Up Tasks

Make initial peers DNS resolution async #1613
Make listener DNS resolution non-blocking #1631
Interrupt handler does not work when a blocking task is running #1351
Windows state path message #1654

@oxarbitrage
Copy link
Contributor Author

In a similar way we can do #1508 for a "LOCK file in use" hint.

I added this at 11cbfc7

Updating the original post.

@oxarbitrage oxarbitrage changed the title Add a hint to port conflict panic Add a hint sto port conflict and lock file panic Dec 17, 2020
@oxarbitrage oxarbitrage changed the title Add a hint sto port conflict and lock file panic Add hints to port conflict and lock file panic Dec 17, 2020
@oxarbitrage oxarbitrage changed the title Add hints to port conflict and lock file panic Add hints to port conflict and lock file panics Dec 17, 2020
@teor2345
Copy link
Contributor

Can you also fix #1509 In this PR?

#1484 and #1509 should have similar hints, so I want to do them in the same PR.

@teor2345 teor2345 self-requested a review December 17, 2020 20:31
@teor2345 teor2345 added A-rust Area: Updates to Rust code C-bug Category: This is a bug labels Dec 17, 2020
@teor2345 teor2345 added this to the v1.0.0-alpha.1 milestone Dec 17, 2020
@teor2345 teor2345 added the S-needs-triage Status: A bug report needs triage label Dec 17, 2020
@teor2345
Copy link
Contributor

It might also be helpful to add an acceptance test that:

  1. launches two Zebra instances with the same temporary directory and random port, and
  2. makes sure that the output includes any one of these hints (it doesn't matter which one)

I added a random_port() function to the acceptance tests a few days ago - you can use it to pick a random port for this test's config.

@oxarbitrage
Copy link
Contributor Author

I can't see how to reproduce #1509. For #1484 i can just open the port with some other program, for #1508 i can run 2 instances of zebrad in different ports.

For #1509 according to logs the error should be originated here https://github.com/ZcashFoundation/zebra/blob/main/zebrad/src/components/sync.rs#L280

@teor2345
Copy link
Contributor

teor2345 commented Dec 17, 2020

I can't see how to reproduce #1509. For #1484 i can just open the port with some other program, for #1508 i can run 2 instances of zebrad in different ports.

For #1509 according to logs the error should be originated here https://github.com/ZcashFoundation/zebra/blob/main/zebrad/src/components/sync.rs#L280

See this comment in #1484:

if you have zcashd occupying 8233 it just panics with zebra_network::peer::connection: e=buffered service failed: buffered service failed: Address already in use (os error 98)

Try disabling the metrics and tracing listeners, and just having a Zcash network protocol listener?

There are 3 different ports that can conflict:

  1. metrics listener (Add a hint to the port conflict panic #1484)
  2. tracing listener (Add a hint to the port conflict panic #1484)
  3. Zcash network protocol listener (Add a hint to the "Address already in use" error #1509)

We need to cover all 3 cases in this PR.

@oxarbitrage
Copy link
Contributor Author

If you have a zcashd running what you get with this PR is:

...
The application panicked (crashed).
Message:  Listener failed. Port already in use. Is another instance of zebrad or zcashd running in your local machine ? Consider changing the default port in the configuration file.
Location: /home/oxarbitrage/zebra/issue1484/zebra/zebra-network/src/peer_set/initialize.rs:218
...

@oxarbitrage
Copy link
Contributor Author

I actually don't see #1484 referring to metrics or tracing endpoints. By the description it seems clear to me that the problem is at port 8233 which is the network port.

@teor2345
Copy link
Contributor

I actually don't see #1484 referring to metrics or tracing endpoints. By the description it seems clear to me that the problem is at port 8233 which is the network port.

Yes, but any of those ports can conflict, so we need to add hints to all of them.

@oxarbitrage
Copy link
Contributor Author

Yes, but any of those ports can conflict, so we need to add hints to all of them.

Makes sense, i will take care of this.

@oxarbitrage
Copy link
Contributor Author

Added hints for tracing and metric endpoints port conflict in abfe4f6 and 73a74ef. When i saw the tracing one i noticed there was a hint as an error and i changed to a panic. I understand the application could continue without the tracing endpoint but it is not what the user will want. I think a panic is fine here but not totally sure.

yaahc
yaahc previously approved these changes Dec 18, 2020
Copy link
Contributor

@yaahc yaahc left a comment

Choose a reason for hiding this comment

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

this looks great ^_^

zebrad/src/application.rs Show resolved Hide resolved
zebrad/src/application.rs Outdated Show resolved Hide resolved
Avoids weird escaping on Windows when using Debug
And add another Windows-specific port error alternative
zebrad/src/application.rs Outdated Show resolved Hide resolved
zebrad/src/application.rs Outdated Show resolved Hide resolved
@mpguerra mpguerra modified the milestones: 2021 Sprint 1, 2021 Sprint 2 Jan 29, 2021
@oxarbitrage
Copy link
Contributor Author

Last changes looks good, thanks @teor2345 , looks like this one is finally ready.

CI failing at large sync tests which is unrelated here, all normal tests passing.

@teor2345
Copy link
Contributor

CI failing at large sync tests which is unrelated here, all normal tests passing.

I updated #1651 to add the error message we saw in this CI run. Still Windows large sync tests, but at a slightly different stage of linking.

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I think we're good to go here.

@teor2345 teor2345 added A-dependencies Area: Dependency file updates C-cleanup Category: This is a cleanup C-enhancement Category: This is an improvement labels Jan 29, 2021
@teor2345 teor2345 merged commit 4b34482 into ZcashFoundation:main Jan 29, 2021
@mpguerra mpguerra mentioned this pull request Apr 11, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependencies Area: Dependency file updates A-rust Area: Updates to Rust code C-bug Category: This is a bug C-cleanup Category: This is a cleanup C-enhancement Category: This is an improvement
Projects
None yet
4 participants