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

Aggregate Publisher/Listener #43

Open
Yortw opened this issue Nov 28, 2016 · 13 comments
Open

Aggregate Publisher/Listener #43

Yortw opened this issue Nov 28, 2016 · 13 comments

Comments

@Yortw
Copy link
Owner

Yortw commented Nov 28, 2016

Many consumers would like to listen/publish on multiple adapters/networks/addresses (whatever is the right term here). Rssdp doesn't support that, and there are architectural issues with trying to implement it given the current design.

To make life easier for consumers, could we; create an AggregateSsdpDevicePublisher (and possibly AggregateSsdpDeviceLocator) which take an IEnumerable of the associated type. Add a device to the aggregate publisher would add it to all internal publishers, and performing a search on the aggregate locator would union the search from all internal locators. Other properties and settings would also map down to the inner collections, allowing consumers to treat multiple instances as if they're a single instance.

There are some potential issues here, for example, with the locator what happens when one inner locator throws an exception on search? Does the user get the exception and no results, or the successful results and the exception is buried?

@Yortw
Copy link
Owner Author

Yortw commented Nov 28, 2016

@LukePulverenti Do you think something like this would be useful to you? I know you said your old code used to listen on multiple interfaces. Any comments on the proposed solution? Thanks.

@Tapanila
Copy link
Contributor

I would like to see a feature where I can just do locate on all adapters. Especially on desktop development computers it is pretty normal to have multiple network adapters, where most of them are virtual either for VMWare, Hyper-V or some other virtualization platform.

This would allow me to just not care about their setup and make sure it just works.

The exception question is bit tricky. My use case would benefit from having the results and burying the exception.

@Yortw
Copy link
Owner Author

Yortw commented Nov 29, 2016

Technically this would allow that, though possibly with more setup than you'd like. Your issue with the error handling is exactly the sort of issue I'm concerned about, especially since I can see some people want it the other way (i.e wanting to know it failed on one adapter and the results are incomplete).

pretty normal to have multiple network adapters, where most of them are virtual either for VMWare, Hyper-V or some other virtualization platform

Yes, and in many cases searching on these adapters is pointless. I have many virtual adapters for VPN's, some for virtualisation etc. and most of these do not support broadcast traffic, even when they do I typically only want to find devices on the local network and not on some remote network accessed via VPN. That's why I think a solution where you choose the adapters (and can choose all if that's your thing) is better than literally seraching everything with no option to do otherwise.

Thanks for the input, especially on the error handling. That is the way I'm leaning, though I wonder if perhaps the results should be a collection of result objects that contain either the result set or an exception. Easy enough to use linq to union the results ignoring the exceptions, but you'd still be able to know if there was an exception on any particular adapter if you cared. Seems like a bit of an ugly interface though, so maybe just burying the exceptions is better. I guess if you care about that level of detail you could go back to doing it manually and not using the aggregate adapter, which is possibly part of what makes that a good pattern.

@LukePulverenti
Copy link
Contributor

Just for context, what has led to this question is we think there might be a situation of some messages not being received when using just IpAddress.Any, at least on Windows. I don't have evidence to prove this yet.

For our use, if this turns out to not be the root problem, then for simplicity we would just stick with IPAddress.Any and just filter messages as they are received.

@Yortw
Copy link
Owner Author

Yortw commented Nov 30, 2016

Would be great to know if IpAddress.Any really does miss some messages (seems odd, seems more likely a bug in RSSDP code). Any info appreciated.

@LukePulverenti
Copy link
Contributor

LukePulverenti commented Dec 4, 2016

Unfortunately I can confirm - there are situations where you need to explicitly bind Ip addresses. I'm now able to reproduce a situation where IpAddress.Any does not receive messages, while a network address does receive them.

I don't believe there is an RSSDP bug filtering out the message. I have put the debugger on and I just don't see them coming in at all. I also don't believe there is an issue with socket configuration. I have tried various socket options every which way and can't seem to resolve it. For me, the only thing that resolves it is a socket bound to a specific address.

Rather than use multiple comm servers, my suggestion is to put this into the comm server so that it can share the same cache. So it would be a list of send sockets rather than a single _SendSocket instance. I will probably whip that up today just to solve my requirement and get started with users testing it.

This now creates another requirement of having to monitor the system for network adapters coming online. I would have preferred to just stay with IpAddress.Any but I don't see another option right now.

Also, to my knowledge this is only necessary on desktop Windows, so I am also going to have this behavior configurable, to make sure we're not opening extra sockets on platforms that don't need it.

@Yortw
Copy link
Owner Author

Yortw commented Dec 4, 2016

So you're prepared to break the public interfaces to do that?

How do you intend to handle the published device address? (According to spec, it specifcally says the device description document address should use the address of the adapter the search request came in on).

In my case I have scenario's where PC's have 1-3 network adapters in them and I only want to publish on one (publishing on the other two is useless/unneccesary, and could cause problems). Does your implementation have a solution?

I presume we need to handle adapters going online and offline, and probably in some threadsafe way, since they could disappear in the middle of a broadcast/search response etc.

(Long shot:) When this problem happen, is the Windows SSDP service running?

Let me know if you get your changes done and I'll take a look to see if I can port them over.

Thanks.

@Yortw
Copy link
Owner Author

Yortw commented Dec 4, 2016

Also, do you have steps to reproduce? Might be helpful for me to be able to test in this situation.

@LukePulverenti
Copy link
Contributor

The Windows SSDP service is running, yes, and it could very well be related to that. Unfortunately I can't document steps to reproduce the problem, since I don't yet know what conditions create it in the first place. I have tried researching IpAddress.Any not receiving socket messages and haven't found much, that gives me some hope that there might be a solution that doesn't require all this, or perhaps this is even further isolated to Windows 10.

As far as responding to search requests, what you should probably do regardless of this is pass that information around with the search events so that the respond methods use that instead of global information. That's something you could do without breaking the current implementation and would also prepare the library to handle multiple sockets if you decide to go that route.

As far as unnecessary adapters go, correct me if I'm wrong, but the current implementation on IpAddress.Any already has that issue anyway, so all I'm really interested in right now is an equivalent replacement. Once this is functional it would be straightforward to allow options to be passed in from the outside to filter if desired.

@LukePulverenti
Copy link
Contributor

I have stubbed out a quick prototype: MediaBrowser/Emby@cf5f70d

It keeps a list of _sendSockets. When a search request comes in the local ip address is recorded and that is used to determine which sockets to send from.

Because we're now dealing with a list of sockets, error handling will be needed to ensure that a socket failure with one of them does not prevent messages from being sent by the others. You'll probably want to add a logging interface into the library so that consuming applications can inject their own logging. Right now I have exceptions printing to the log in order to help troubleshoot.

This is just a first pass so I don't expect it to be perfect right now. I do still plan to add OS filtering so that we can just stick to IpAddress.Any on platforms where this workaround is not required. It would just be a check in CreateSocketAndListenForResponsesAsync.

@Yortw
Copy link
Owner Author

Yortw commented Dec 6, 2016

Thanks, I'll take a look at the prototype.
Sorry I expressed myself poorly over the aggregate vs multiple socket issue. I was attempting to determine if your solution would allow for multiple sockets but not all sockets. Of course it could, it's just whether it does or not.

The address for a service could (even if it's not likely) vary by more than just the adapter address, so I'm not sure just pasing the address the search came from is sufficient. There's also the issue of how to correctly replace the default address any way (use Uri to parse the address, change the authority and then rebuild the uri?).

@Yortw
Copy link
Owner Author

Yortw commented Dec 6, 2016

Have been thinking about adding some proper logging for a while, but not sure how. There's the common logging abstraction library, but I don't really want to add the dependency. I can add my own interface (or use a func<> of some sort), but then everyone has to adapt it to whatever library they're using, and what exactly should the interface look like? Advice appreciated.

Your example also uses your INetworkManage interface/implementation which I don't have. Obviously I could build something similar and then have you adapt yours to it, but since RSSDP has somewhat simpler needs how about if we just provided an obserable list of some sort? ObservableCollection or similar? RSSDP can use the initial list of sockets, and watch for sockets being added/removed. That leaves all the complexity of how/when/if to monitor the network changes to the caller (so your NetworkManager implementation) and RSSDP just cares about the address list changing. Seems cleaner, though does off load more work to the caller, but given the requirements & code might change per app/platform that seems like a good separation?

@Yortw
Copy link
Owner Author

Yortw commented Dec 21, 2016

@LukePulverenti I need to keep platform support for some older platforms (in particular .Net 4). The portable class library target for 4.0 doesn't support ObservableCollection (nor INotifyCollectionChanged to re-implement myself). Given we need to be able to add and remove sockets during the lifetime of the publisher, it seems I either need;

  1. My own observable list/collection of some kind (incompatible with the .net standard)
  2. Some kind of add/remove or bind/unbind methods on the publisher itself
  3. Something like your network manager, but given all I really need is a list of addresses or sockets to bind to that I can monitor for changes, it seems cleaner to just use some kind of observable collection.'

Do you have a preference or thoughts on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants