httpu: add context.Context and related interface #61
Conversation
httpu/multiclient.go
Outdated
| var ctxDelegates []ClientInterfaceCtx | ||
| for _, del := range mc.delegates { | ||
| if cd, ok := del.(ClientInterfaceCtx); ok { | ||
| ctxDelegates = append(ctxDelegates, cd) | ||
| } else { | ||
| return nil, fmt.Errorf("client does not implement ClientInterfaceCtx") | ||
| } | ||
| } |
There was a problem hiding this comment.
This is ugly because we have to check at runtime whether the mc.delegates members implement the ClientInterfaceCtx interface; if we put DoWithContext in the main ClientInterface interface, then we can avoid this.
There was a problem hiding this comment.
Is there a compelling case to have ClientInterfaceCtx include ClientInterface? If not, then we can have two separate MultiClient implementations, one of which takes contexts.
There was a problem hiding this comment.
I think that MultiClient is only used (instantiated) by goupnp.httpClient(), which in turn is only used by goupnp.DiscoverDevicesCtx(), which ideally would pass its ctx into it.
So we could keep compatibility by having a context-respecting version of MultiClient, and also have DiscoverDevicesCtx use the context in one extra place.
There was a problem hiding this comment.
I started working on this, but DiscoverDevicesCtx calls ssdp.SSDPRawSearchCtx with the httpClient, so in order to get the Context behaviour, we'll either have to change the signature, or add another method that does the ~same thing but takes a ClientInterfaceCtx. Thoughts?
There was a problem hiding this comment.
Argh, that's a good point. Another function isn't too bad though, and could be named more simply RawSearch, which removes the ssdp.SSDPRawSearch stutter.
There was a problem hiding this comment.
Okay, pushed an approach that splits up the interface, and adds the aforementioned function. What do you think?
(also, separately: I've got an example using NewWANIPConnection2ClientsCtx that I've been using for testing since my router doesn't support NewWANPPPConnection1Clients; any interest in me adding that example to this PR, or as a follow-up?)
There was a problem hiding this comment.
Thanks, I'll try to take a look at this this evening (UK time).
As to your second question: that sounds like it should be a separate PR, but sounds good.
andrew-d
force-pushed
the
andrew/do-with-ctx
branch
from
981f6b1
to
b0a0064
Compare
ssdp/ssdp.go
Outdated
| defer cancel() | ||
| } | ||
|
|
||
| // Check the timeout on the context (if any); if the context would time |
There was a problem hiding this comment.
This line through and including the request creation (lines 111-130) look like they are common code that can be factored out (func prepareRequest(...) (*http.Request, error) ?).
Or could SSDPRawSearchCtx be implemented mostly by calling RawSearch?
There was a problem hiding this comment.
I factored them out into a shared function. I didn't want to change the behaviour of SSDPRawSearchCtx since users may have been expecting that the Do function would be called, and RawSearch calls DoWithContext instead (and requires a different interface type).
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
force-pushed
the
andrew/do-with-ctx
branch
from
b0a0064
to
ab5fddf
Compare
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 #55