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

Write deadlock fixes and overall reliability improvements #30

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

requilence
Copy link

@requilence requilence commented Mar 3, 2023

  1. introduce writeTimeout for both server and clients
    I have noticed that in some scenarios socket write(like here) can hang for indefinite time.
    At least it reproducible in the latest MacOS release(13.2) when writing on some standard network interfaces.
    In this case it blocks all the further writes to other interface and deadlock Shutdown
    This PR introduce WriteTimeout and ClientWriteTimeout option with default of 10 sec. It seems to be much more than ok. In my case it either writes under 10ms or hangs indefinitely

Overall if it will be timeout'ed we still have 3 tries in the probe

  1. Introduce the proxy struct NetInterface that alongside the net.Interface stores the bitmask flags MulticastJoined and MessageSent separately for IPv6 and IPv4.
    The reason behind this is that multicastResponse call without interface specified works in a way it iterates through all interface to send the message. This makes unregister tries to connect to interfaces that can be failed to join multicast group or send the announce - which is make no sense

On macOS I faced situations where some interface can join IPv6 multicast but can't join IPv4 multicast, or can announce on IPv6 but always hang(timeout) during announceText.

So multicastResponse now has an optional arguments to pass required flags

  1. Add ServerSelectIPTraffic option similar to client's SelectIPTraffic option to disable IPv6 or IPv4 multicast.
    This seems pretty useful to have, because you have no other way to control it, and IPv6 multicast may be more tricky to support

@MarcoPolo MarcoPolo self-requested a review March 6, 2023 19:16
@MarcoPolo
Copy link

Do these issues also show up in the https://github.com/grandcat/zeroconf library that this fork is based on? If so I think it work benefit more folks to open the PR there.

@MarcoPolo
Copy link

Also, could it be possible to add some tests here as well?

@MarcoPolo MarcoPolo added the need/author-input Needs input from the original author label May 1, 2023
@MarcoPolo
Copy link

friendly ping @requilence

@MarcoPolo MarcoPolo removed their request for review May 22, 2023 20:17
@p-shahi
Copy link
Member

p-shahi commented May 30, 2023

Hi @requilence , go-libp2p maintainers are not prioritizing reviewing this atm. It would be great if you can split this PR into two: one for bugfixes and one for the reliability improvements. We can prioritize looking at the bugfix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/author-input Needs input from the original author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants