weixin_39963174
2020-12-02 14:14 阅读 10

Information hiding for JmDNS

The usage of JmDNS is an implementation detail and should not be exposed to the API

This breaks the API a little, but I think it's worth it. The JmDNS object exposes how the discovery is done. This should stay the implementation secret of the library. What do you think?

该提问来源于开源项目:vitalidze/chromecast-java-api-v2

  • 点赞
  • 写回答
  • 关注问题
  • 收藏
  • 复制链接分享

12条回答 默认 最新

  • weixin_39624071 weixin_39624071 2020-12-02 14:14

    I think it is ok to change it in this case. LGTM! Thanks!

    点赞 评论 复制链接分享
  • weixin_39704374 weixin_39704374 2020-12-02 14:14

    I don't know if our implementation is flawed - I don't own a Chromecast so I haven't looked into this, but this API change breaks our implementation: UniversalMediaServer/UniversalMediaServer#1197

    点赞 评论 复制链接分享
  • weixin_39624071 weixin_39624071 2020-12-02 14:14

    that's correct, it is a breaking change. However, your code may be changed a little to adapt to these changes.

    点赞 评论 复制链接分享
  • weixin_39704374 weixin_39704374 2020-12-02 14:14

    It seems more than a lille to me. Please give me a hint of how to easily adapt this. It seems we run our own JmDNS. I know nothing about our implementation of Chromecast, but we have to keep tracks of all devices ourselves for other reasons. We also deal with a lot of multicast devices, although most don't use DNS - it might be necessary for us to run a multicast DNS for other purposes. I'm not so sure it's a good idea to run two of those - thus I'm not sure it's smart to "lock down" the discovery service like this.

    We could obviously solve it by reflection or forking, but that kind of defeats the purpose of OO and dependencies.

    点赞 评论 复制链接分享
  • weixin_39624071 weixin_39624071 2020-12-02 14:14

    use ChromeCasts instead of JmDNS. I think you can put it directly to the ChromeCastMgr. Then in ChromeCasts class there is a way to subscribe for discovery events with the registerListener(ChromeCastsListener) static method. You can then replace implementation of JmDNS ServiceListener interface with implementation of ChromeCastsListener interface. To start discovery just call startDiscovery method in ChromeCasts class. This will leave you safe if future implementation will switch from JmDNS to something different. Also I believe the code will be more simple in this case.

    点赞 评论 复制链接分享
  • weixin_39704374 weixin_39704374 2020-12-02 14:14

    We're cross-posting a little here 😉 There's no need for you to post a PR, but I'm still not sure that it's a good idea that

    ChromeCasts
    "owns" the
    JmDNS
    instance or that you should claim general network services as your own in a
    final
    class with everything
    private
    . In fact, I'm not sure such paranoia is ever useful.
    ChromeCasts
    is a very simple class and should be no problem for others to extend.

    Failing to do that, "parallel" implementations are inevitable - our

    ChromecastMgr
    is a parallel implementation that could have been a subclass of
    ChromeCasts
    instead if it were extendable.

    I see now that it's not that big a problem, as I can "mimic" the hidden constructor by using setters. However, you should at the very least offer an alternative constructor like this that IS implementation agnostic:

    java
        ChromeCast(String name, String address, int port, String appsURL, String application) {
            this.name = name;
            this.address = address;
            this.port = port;
            this.appsURL = appsURL;
            this.application = application;
        }
    
    点赞 评论 复制链接分享
  • weixin_39624071 weixin_39624071 2020-12-02 14:14

    let's continue discussion here.

    I'm still not sure that it's a good idea that ChromeCasts "owns" the JmDNS instance or that you should claim general network services as your own in a final class with everything private. In fact, I'm not sure such paranoia is ever useful. ChromeCasts is a very simple class and should be no problem for others to extend.

    First of all me personally prefer composition instead of inheritance.

    Then, again, JmDNS is an implementation detail, which may change in future. I don't get what do you mean by "you should claim general network services as your own in a final class with everything private". This is a public class, which hides implementation and provides some set of public methods for the end user.

    JmDNS is used for discovery purpose only, if you need it somewhere outside of "ChromeCast" module, then I don't see any reason why you can't use it. I just want ChromeCast module or any other application using the library to not depend on it directly. This means you don't need to know anything about JmDNS to use the library, it provides everything needed, at least from my point of view.

    Failing to do that, "parallel" implementations are inevitable - our ChromecastMgr is a parallel implementation that could have been a subclass of ChromeCasts instead if it were extendable.

    I don't understand the reason to do it. And I think you can easily use composition here if necessary.

    I see now that it's not that big a problem, as I can "mimic" the hidden constructor by using setters. However, you should at the very least offer an alternative constructor like this that IS implementation agnostic:

    This sounds reasonable. I think I will add it in newer version of the library.

    点赞 评论 复制链接分享
  • weixin_39704374 weixin_39704374 2020-12-02 14:14

    Sorry, I meant to write "you shouldn't claim general network services as your own in a final class with everything private". By that I mean that a general network service, such as multicast DNS should be shared by all components in an application that need to use it. It's bad practice and can often lead to problems if an application has to run several instances of such services. Therefore, it should be possible to use an "application global" multicast DNS service for discovery of multicast devices.

    If

    ChromeCasts
    was possible to extend it would be possible to implement a subclass that utilized a shared service, but as it is now that's not possible.

    First of all me personally prefer composition instead of inheritance.

    I think this article cover the topic well so I don't have to write a lot on this here, but I disagree: They are completely different things. By "locking a class down" so that inheritance is impossible, you're taking away that option in the cases where that is appropriate. An author can always choose to use composition if that's what fits best in a given scenario even if subclassing is possible.

    Then, again, JmDNS is an implementation detail, which may change in future. I don't get what do you mean by "you should claim general network services as your own in a final class with everything private". This is a public class, which hides implementation and provides some set of public methods for the end user.

    Using

    JmDNS
    is an implementation detail, but using multicast DNS is not. It's fundamental to Chromecast and many other devices. If you want the class to be
    JmDNS
    independent, you would need to redesign it so that it is actually multicast implementation agnostic and then provide an implementation of that which use for example
    JmDNS
    .

    JmDNS is used for discovery purpose only, if you need it somewhere outside of "ChromeCast" module, then I don't see any reason why you can't use it. I just want ChromeCast module or any other application using the library to not depend on it directly. This means you don't need to know anything about JmDNS to use the library, it provides everything needed, at least from my point of view.

    JmDNS

    is private and isn't exposed by any methods, so it isn't available for use outside

    ChromeCasts

    . The only option is to run several multicast DNS instances, which is what I'm arguing against. Alternatively, one has to implement a similar class to

    ChromeCasts

    that does more or less the same but can do the additional stuff needed and use a shared instance. That's what I mean by "parallell implementation". You end up not using

    ChromeCasts

    at all because of it's inflexibility.

    I don't understand the reason to do it. And I think you can easily use composition here if necessary.

    Again I can't if I want to use the multicast DNS instance for other purposes as well.

    But, by all means, this isn't that important to me - I'm used to having to use different "hacks" like reflection and parallel implementations because libraries are too restrictive in their designs. I think Java has some flaws in it's OO model and I particularly hate "package private". As I see it there should be really good reasons for not using

    protected
    for most things, and
    final
    classes should be even more rare.

    I can see that it's often easier for the author to hide everything away because that means the author doesn't have to think through the design and can refactor it at will. But, it also basically removes most of the benefits of OO.

    点赞 评论 复制链接分享
  • weixin_39624071 weixin_39624071 2020-12-02 14:14

    sorry for delay in response.

    By that I mean that a general network service, such as multicast DNS should be shared by all components in an application that need to use it. It's bad practice and can often lead to problems if an application has to run several instances of such services.

    I don't understand what's wrong with using multiple multicast DNS listeners within same application? What is the difference between multiple applications with single listener, and one application with multiple instances? Can you provide some info on the issues you will have in such case? The only thing here from my point of view is consuming of machine resources. This can be the case, but who are talking about running hundreds/thousands of such service listeners?

    If ChromeCasts was possible to extend it would be possible to implement a subclass that utilized a shared service, but as it is now that's not possible.

    Currently ChromeCasts is meant to be used as "singleton".

    I think this article cover the topic well so I don't have to write a lot on this here, but I disagree: They are completely different things. By "locking a class down" so that inheritance is impossible, you're taking away that option in the cases where that is appropriate. An author can always choose to use composition if that's what fits best in a given scenario even if subclassing is possible.

    The main reason to "lock a class down" is to force correct usage of the library. Libraries usually define points of extensions, and if the class/constructor are hidden, then you should not use. Otherwise you will be shooting your own leg. No one will guarantee that your code will not break with the future release of the library.

    Using JmDNS is an implementation detail, but using multicast DNS is not. It's fundamental to Chromecast and many other devices. If you want the class to be JmDNS independent, you would need to redesign it so that it is actually multicast implementation agnostic and then provide an implementation of that which use for example JmDNS.

    I think that defining an high level mDNS APIs is over-engineering for now.

    Again I can't if I want to use the multicast DNS instance for other purposes as well.

    You can use it, no one can stop you. Please provide some arguments why you cannot have two instances inside your own app.

    But, by all means, this isn't that important to me - I'm used to having to use different "hacks" like reflection and parallel implementations because libraries are too restrictive in their designs. I think Java has some flaws in it's OO model and I particularly hate "package private". As I see it there should be really good reasons for not using protected for most things, and final classes should be even more rare.

    Well, I have one important question here. Are you sure that first implementation of the ChromeCast support in "UMS" was targeted on sharing jmDNS instance? Currently all this dispute is about sharing, which is not the case, it is not shared anyhow. Yes, it can be shared, but currently it is not. From my understanding the first implementation was not about sharing, but about discovery. First versions of this library didn't provide any listeners for the event of discovery of chrome cast device. That's why the only option was to implement mDNS listener and then construct ChromeCast instances manually. In newer versions, the situation improved and I am trying to tell you the correct way to implement it for UMS. But instead of understanding this, you are trying to force some ideas about sharing mDNS instances. Then you are using some hacks, implementing your own jmDNS listeners, using reflections and so on. I am a bit lost in the big reason about all that.

    I can see that it's often easier for the author to hide everything away because that means the author doesn't have to think through the design and can refactor it at will. But, it also basically removes most of the benefits of OO.

    You should use library the way it is designed to be used. The main goal of such restrictions is to lead user to that correct way. It can be treated as "instruction" of the proper use. Usually you don't need to know about anything internal of the library and just use it as black box with the public APIs and extension points that are provided. I don't see any flaws with OO design here. Can you provide some successful example about extending a "core" component of a library?

    点赞 评论 复制链接分享
  • weixin_39704374 weixin_39704374 2020-12-02 14:14

    I don't understand what's wrong with using multiple multicast DNS listeners within same application? What is the difference between multiple applications with single listener, and one application with multiple instances? Can you provide some info on the issues you will have in such case? The only thing here from my point of view is consuming of machine resources. This can be the case, but who are talking about running hundreds/thousands of such service listeners?

    I don't know what the potential consequences of running multiple mDNS clients, but I always try to avoid such solutions if it's possible. I've had enough network issues that can be very time consuming to track down in my time to gamble that it's safe when it's so easy to prevent. I'd have to read the spec to be sure about the details, but they will listen to the same socket and keep contacting each other. I see a lot of potential for side effects there. It's nothing that says that it's safe to run multiple instances in multiple applications on the same computer either - it might be, but I wouldn't be surprised if it turned out to have issues. To me it's as simple as: Why take the chance when you can be safe and run only one instance?

    Currently ChromeCasts is meant to be used as "singleton".

    There's no contradiction between being a singleton and being extendable. That said, I don't think the singleton model you've chosen is very fortunate. First of all, it's initialized when the class loads, which means that it might start without being needed by the application. Second and worse, it's not thread-safe. And because it's final and the methods are private there's no way to make it thread-safe either. The only safe option here is to never reference the class at all so that the static singleton is never constructed. Using this kind of singleton is also bad for testing. You should look at Thread Safe Singleton here instead.

    The main reason to "lock a class down" is to force correct usage of the library. Libraries usually define points of extensions, and if the class/constructor are hidden, then you should not use. Otherwise you will be shooting your own leg. No one will guarantee that your code will not break with the future release of the library.

    That might be the reasoning behind it, but it's important to realize that the author of given library can't anticipate the needs of the application using it. You might have one use-case in mind where you consider this the "correct usage", but forcing that as the only way might mean that others cannot use the library at all, or have to make a fork to make it work. Isn't it better to make it usable for as many as possible? You shouldn't assume that those that extend a class is that ignorant that you think extending it is "shooting your own leg", especially for a class as simple as this. Anyone extending a class will have the responsibility that it's working as intended, not the author of the super class, so that shouldn't be something you'd worry about. There are never any guarantees that code won't break with new releases, but it's a good practice to try to avoid it, and if it's unavoidable it should be indicated via the version number and be described in release notes.

    I think that defining an high level mDNS APIs is over-engineering for now.

    I agree, but you shouldn't hide the "implementation detail" as long as that is missing. Let's face it - in this situation, switching mDNS library wouldn't mean you'd have to remove the currently hidden constructor. It's only used to initalize fields anyway, so you could just define a new constructor for the new implementation and leave that constructor in place.

    You can use it, no one can stop you. Please provide some arguments why you cannot have two instances inside your own app.

    See further up.

    Well, I have one important question here. Are you sure that first implementation of the ChromeCast support in "UMS" was targeted on sharing jmDNS instance? Currently all this dispute is about sharing, which is not the case, it is not shared anyhow. Yes, it can be shared, but currently it is not. From my understanding the first implementation was not about sharing, but about discovery. First versions of this library didn't provide any listeners for the event of discovery of chrome cast device. That's why the only option was to implement mDNS listener and then construct ChromeCast instances manually. In newer versions, the situation improved and I am trying to tell you the correct way to implement it for UMS. But instead of understanding this, you are trying to force some ideas about sharing mDNS instances. Then you are using some hacks, implementing your own jmDNS listeners, using reflections and so on. I am a bit lost in the big reason about all that.

    You are correct that the current implementation in UMS doesn't share the mDNS instance, so to get the same functionality as we have today we could just subscribe to the events available in

    ChromeCasts
    . I'm thinking ahead and see that we probably should/have to implement a shared mDNS in the future. I'm also simply stating that I think the design should be different, not that this is a big problem. I didn't write the current UMS implementation, and our implementation isn't thread-safe either - a situation I'm generally trying to rectify many places in the code. I'm not saying that our
    ChromecastMgr
    is a better solution, I'm simply saying that I see issues with the library design. There's no reason for me to tell you about the issues with our implementation, that's our responsibility.

    You should use library the way it is designed to be used. The main goal of such restrictions is to lead user to that correct way. It can be treated as "instruction" of the proper use. Usually you don't need to know about anything internal of the library and just use it as black box with the public APIs and extension points that are provided. I don't see any flaws with OO design here. Can you provide some successful example about extending a "core" component of a library?

    This is partly covered above. The main issue I have with your philosophy here is that you think there is "a correct way". You're basicly saying "Use it as I've decided or bugger off". That's your right. My idea of library design is that you want to make it as versatile as possible so that as many applications as possible can utilize it. I simply think it makes more sense, in a way you get more "usefulness" out of every hour you put into the project.

    When it comes to extending "core" components of a library - I'm not sure how you define "core" here, but

    ChromeCasts
    is typically a class you'd want to extend to apply your application-specific logic and rules to the discovery and tracking of devices. I'm extending library classes "all the time", and for many libraries it's a breeze because they have been designed so that it's possible. Actually where I've had the most trouble is with the core Java library and Swing. Swing has a lot of really stupid OO design issues that makes customizing it to work a certain way (or simply to fix a bug) can be very hard.

    As to examples, I don't know the relevance, but I've recently extended Metadata-extractor, Thumbnailator, ImageIO, LogBack and Cling in addition to many "core" Java and Swing classes. In some cases, it I think my extensions might have value to the library and it has a realistic chance to be merged, I've generalized my extended code and created a PR. There's nothing unusual about that as I see it, we're all sharing code to make it as good and versatile as possible.

    点赞 评论 复制链接分享
  • weixin_39624071 weixin_39624071 2020-12-02 14:14

    I don't know what the potential consequences of running multiple mDNS clients, but I always try to avoid such solutions if it's possible. I've had enough network issues that can be very time consuming to track down in my time to gamble that it's safe when it's so easy to prevent. I'd have to read the spec to be sure about the details, but they will listen to the same socket and keep contacting each other. I see a lot of potential for side effects there. It's nothing that says that it's safe to run multiple instances in multiple applications on the same computer either - it might be, but I wouldn't be surprised if it turned out to have issues. To me it's as simple as: Why take the chance when you can be safe and run only one instance?

    From my understanding the ChromeCast is designed for multiple consumers/senders from the start. This means that on the same network there will be many mDNS listeners. Isn't it a cheap device that you can install everywhere, on TV sets, on audio receivers, etc. ? Same goes for multicast sockets. Thus, for me it looks like a fresh, lightweight and scalable way of services discovery, which can be widely used without any fear.

    There's no contradiction between being a singleton and being extendable. That said, I don't think the singleton model you've chosen is very fortunate. First of all, it's initialized when the class loads, which means that it might start without being needed by the application. Second and worse, it's not thread-safe. And because it's final and the methods are private there's no way to make it thread-safe either. The only safe option here is to never reference the class at all so that the static singleton is never constructed. Using this kind of singleton is also bad for testing. You should look at Thread Safe Singleton here instead.

    I agree about your clause with singleton limits extension here. And that's what this whole dispute is about, that by my means you should not extends this class :) Regarding thread safety, the eager initialization, which is being used is thread safe. Even same site that you mentioned tells exactly same thing. It is not lazy, as you correctly state that it is initialized during class-loading, this is fact, which cannot be discussed further. The class may be not thread-safe, I can see only listeners collection which can break the flaw. Others looks good for me. Am I wrong?

    That might be the reasoning behind it, but it's important to realize that the author of given library can't anticipate the needs of the application using it. You might have one use-case in mind where you consider this the "correct usage", but forcing that as the only way might mean that others cannot use the library at all, or have to make a fork to make it work. Isn't it better to make it usable for as many as possible? You shouldn't assume that those that extend a class is that ignorant that you think extending it is "shooting your own leg", especially for a class as simple as this. Anyone extending a class will have the responsibility that it's working as intended, not the author of the super class, so that shouldn't be something you'd worry about. There are never any guarantees that code won't break with new releases, but it's a good practice to try to avoid it, and if it's unavoidable it should be indicated via the version number and be described in release notes.

    That's a good argument, then go and describe your use case. This is why the project is open source, and actually it is open for PRs, discussions, etc. I have publicly available email, issue tracker is public. No one stops any library user from describing his use case. I can even release new version quite fast, within half of day, if it is really needed.

    This is partly covered above. The main issue I have with your philosophy here is that you think there is "a correct way". You're basicly saying "Use it as I've decided or bugger off". That's your right. My idea of library design is that you want to make it as versatile as possible so that as many applications as possible can utilize it. I simply think it makes more sense, in a way you get more "usefulness" out of every hour you put into the project.

    I think that for this kind of library such philosophy is applicable. It does some low-level things undercover and is targeted for being something like an I/O helper for working with ChromeCast device. From my vision it is like an implementation of HTTP client or HTTPURLConnection. Basically, you just use it and you don't need to know how it works and how to extend it. It simply provides you a set of parameters and a public API.

    When it comes to extending "core" components of a library - I'm not sure how you define "core" here, but ChromeCasts is typically a class you'd want to extend to apply your application-specific logic and rules to the discovery and tracking of devices. I'm extending library classes "all the time", and for many libraries it's a breeze because they have been designed so that it's possible. Actually where I've had the most trouble is with the core Java library and Swing. Swing has a lot of really stupid OO design issues that makes customizing it to work a certain way (or simply to fix a bug) can be very hard.

    I agree that ChromeCasts class is not a best example of a "core" class here, it's more like a wrapper used for discovery. By "core" class, for example I can suggest a SessionImpl class from hibernate library. I cannot imagine why anyone would need to extends basic functionality of such complicated class. Though you can provide your own implementation, but I haven't really ever seen it myself.

    As to examples, I don't know the relevance, but I've recently extended Metadata-extractor, Thumbnailator, ImageIO, LogBack and Cling in addition to many "core" Java and Swing classes. In some cases, it I think my extensions might have value to the library and it has a realistic chance to be merged, I've generalized my extended code and created a PR. There's nothing unusual about that as I see it, we're all sharing code to make it as good and versatile as possible.

    I am talking about inheritance, which you are blaming me for. Please provide some particular example.

    点赞 评论 复制链接分享
  • weixin_39704374 weixin_39704374 2020-12-02 14:14

    From my understanding the ChromeCast is designed for multiple consumers/senders from the start. This means that on the same network there will be many mDNS listeners. Isn't it a cheap device that you can install everywhere, on TV sets, on audio receivers, etc. ? Same goes for multicast sockets. Thus, for me it looks like a fresh, lightweight and scalable way of services discovery, which can be widely used without any fear.

    I don't doubt that the ChromeCast can handle multiple "communication partners", but I'm not following you on "Same goes for multicast sockets". As I said before, I can't know for sure because I haven't studied the details, but one possible issue I can imagine is the confusion about the source of messages when multiple listeners are listening to the same socket. Since all listeners will receive all packages, all the listeners on a given socket will receive the packets they send themselves. To prevent handling packages sent by itself, there will have to be some source IP based filtering taking place either in the multicast DNS implementation or in the IP stack. There is no way for this filtering (as far as I can tell) to tell packages coming from different instances apart. That means that it probably will filter out and fail to reply to messages coming from another instance on the same socket. Again, I'm not saying it will cause problems - but why risk it when it's so easily preventable?

    I agree about your clause with singleton limits extension here. And that's what this whole dispute is about, that by my means you should not extends this class :) Regarding thread safety, the eager initialization, which is being used is thread safe. Even same site that you mentioned tells exactly same thing. It is not lazy, as you correctly state that it is initialized during class-loading, this is fact, which cannot be discussed further. The class may be not thread-safe, I can see only listeners collection which can break the flaw. Others looks good for me. Am I wrong?

    I'm not sure if I understand you correct, but what I meant to say was that because a class is a singleton doesn't mean it can't be extended. You can extend the class and then run the extended class instead of the super class, as a singleton.

    I'm sorry if I was unclear, the eager initialization itself is thread-safe (although unfortunate in other ways), but the rest of the class isn't. Neither

    listeners
    nor
    mDNS
    is protected and both can be manipulated by different threads. From a quick look inside JmDNS it seems that class is thread-safe, but the initialization done in
    doStartDiscovery()
    is not.

    That's a good argument, then go and describe your use case. This is why the project is open source, and actually it is open for PRs, discussions, etc. I have publicly available email, issue tracker is public. No one stops any library user from describing his use case. I can even release new version quite fast, within half of day, if it is really needed.

    I've already descibed a case, the need to have an application global multicast DNS instance, the need for special handling and rules for registration of devices (it's not certain that all devices should be allowed to register for example). In addition there's network configuration, even though this isn't implemented in our

    ChromecastMgr
    either (which is a bug) we have network configuration where the user can define which interfaces or IP addresses to listen to.

    I think that for this kind of library such philosophy is applicable. It does some low-level things undercover and is targeted for being something like an I/O helper for working with ChromeCast device. From my vision it is like an implementation of HTTP client or HTTPURLConnection. Basically, you just use it and you don't need to know how it works and how to extend it. It simply provides you a set of parameters and a public API.

    HTTP components are actually something UMS extends a lot. That's the whole point of extending - you need some custom logic that's not part of standard behaviour, or it's simply the easiest way to "hook into" the code at the places you need to.

    I agree that ChromeCasts class is not a best example of a "core" class here, it's more like a wrapper used for discovery. By "core" class, for example I can suggest a SessionImpl class from hibernate library. I cannot imagine why anyone would need to extends basic functionality of such complicated class. Though you can provide your own implementation, but I haven't really ever seen it myself.

    I'm not familiar with the library or the class - or the reasoning for making it final, but

    SessionImpl
    is an extension of
    AbstractSessionImpl
    and implements several interfaces, so one can replace it with another implementation. I'm not saying there never is a reason to make a class final, I'm just saying that it's rare. Whether I think making
    SessionImpl
    final is a good or bad decision I can't say without being familiar with the implementation.

    I am talking about inheritance, which you are blaming me for.

    Sorry, I don't understand. I'm not blaming you for anything - I've described how I think

    ChromeCasts
    should be implemented. You do as you want, obviously.

    Please provide some particular example.

    What do you want, source code? Is this what you're looking for?

    点赞 评论 复制链接分享

相关推荐