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

feat: context everywhere and non-blocking results #55

Open
betamos opened this issue Dec 21, 2022 · 2 comments
Open

feat: context everywhere and non-blocking results #55

betamos opened this issue Dec 21, 2022 · 2 comments
Labels
contrib For issues likely only addressed by contribution. feature Feature request

Comments

@betamos
Copy link

betamos commented Dec 21, 2022

First, thanks for this lib, it's quite amazing how much goes into something like this.

I'm planning to use it for my project for (surprise) port-mapping, and ran some quick experiments to see how it'd fit. I'll be taking different actions depending on status, and in some cases I'll use short lease time.

Issue: The lack of context (or a Close() call) makes it hard to quickly interrupt and shut down ongoing client operations. My project uses a lot of goroutines and I'm really meticulous about teardown logic. Would it be possible to add context to all remaining client calls, such as httpu?

Issue: Even without context, my understanding is that the search functions cannot return until the timeout was reached, even if available sooner. Perhaps this is only true for certain calls like ssdp.SSDPRawSearch? Note: I ran the experiments with the wrapper lib libp2p/go-nat. I'm planning to solicit quick responses and use a fallback otherwise, so any delay will also delay the fallback. Would it be possible to return search results as they come available instead of all-at-once?

Question: The 2s default timeout on SSDP seems like a lot when dealing with local-only connections. How was this determined? Do you have any real-life data points? I may want to set a different default timeout, and I'm curious on how to set it best for my purposes.

I'm happy to contribute if needed, and thanks again.

@betamos
Copy link
Author

betamos commented Dec 22, 2022

Turns out I used an older version of the library. Context does indeed exist in the SSDP search API, although it doesn't appear to be respected in the httpu client, which still runs for its full duration even if the context is canceled. I added the following to the Do method:

	ctx, cancel := context.WithTimeout(req.Context(), timeout)
	defer cancel()
	go func() {
		<-ctx.Done()
		httpu.conn.SetDeadline(time.Now())
	}()

This terminated the httpu call on time, which is I believe the way context is supposed to work. Would something like that be considered a bug fix?

As for shortening the search period: I thought we could use timeout contexts for searches, but this won't work: afaik there are more calls after to get the "device info" (don't know the details of upnp - apologies). If we cancel the context to stop the search, we can't make those requests. For that, we still need to either (a) stream the results as they come available or (b) keep using a timeout (but perhaps we can make it more granular?). Context can still be used as a last resort to terminate everything quickly on teardown.

I also tried measuring the results on my Netgear home router, and it was very fast (1-3ms). Even if the SSDP protocol requires MX >= 1, I'd really want to act on those early results, especially if that's the common case.

@huin
Copy link
Owner

huin commented Dec 22, 2022

This seems like a good request. We'd need to retain the existing API for any code using it, perhaps:

  1. Adding a DoWithContext method that otherwise acts like Do, but provides a way to pass back results as they come in, and respects the context completion.
  2. And then Implement Do in terms of DoWithContext so that there's only one implementation.

@huin huin added feature Feature request contrib For issues likely only addressed by contribution. labels Feb 25, 2023
andrew-d added a commit to andrew-d/goupnp that referenced this issue Aug 22, 2023
This adds a new interface for httpu that supports a Context, and uses
that context to set a deadline/timeout and also cancel the request if
the context is canceled. Additionally, implement the Do method in terms
of the DoWithContext method for the HTTPUClient implementation.

Updates huin#55

Signed-off-by: Andrew Dunham <andrew@du.nham.ca>
andrew-d added a commit to andrew-d/goupnp that referenced this issue Aug 24, 2023
This adds a new interface for httpu that supports a Context, and uses
that context to set a deadline/timeout and also cancel the request if
the context is canceled. Additionally, add a new method to the SSDP
package that takes a ClientInterfaceCtx.

Updates huin#55

Signed-off-by: Andrew Dunham <andrew@du.nham.ca>
andrew-d added a commit to andrew-d/goupnp that referenced this issue Aug 29, 2023
This adds a new interface for httpu that supports a Context, and uses
that context to set a deadline/timeout and also cancel the request if
the context is canceled. Additionally, add a new method to the SSDP
package that takes a ClientInterfaceCtx.

Updates huin#55

Signed-off-by: Andrew Dunham <andrew@du.nham.ca>
huin pushed a commit that referenced this issue Aug 29, 2023
This adds a new interface for httpu that supports a Context, and uses
that context to set a deadline/timeout and also cancel the request if
the context is canceled. Additionally, add a new method to the SSDP
package that takes a ClientInterfaceCtx.

Updates #55

Signed-off-by: Andrew Dunham <andrew@du.nham.ca>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contrib For issues likely only addressed by contribution. feature Feature request
Projects
None yet
Development

No branches or pull requests

2 participants