Uploaded image for project: 'MINA'
  1. MINA
  2. DIRMINA-539

NioDatagramConnector doesn't takes the TrafficClass value set to his DatagramSessionConfig

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Won't Fix
    • Affects Version/s: 2.0.0-M1
    • Fix Version/s: 2.0.8
    • Component/s: None
    • Labels:
      None
    • Environment:
      WinXP, RHEL5 (probably not important)

      Description

      client sending datagrams without taking care to the trafficClas set in the config, so the ToS byte is not set in the packet sent from client.

      client code:

        NioDatagramAcceptor acceptor = new NioDatagramAcceptor();
        DatagramSessionConfig dcfg = ((NioDatagramAcceptor)acceptor).getSessionConfig();
        dcfg.setTrafficClass(tosByte);
        InetSocketAddress bindAddrPort  = new InetSocketAddress(originatingIP, port);
        acceptor.bind(bindAddrPort);
      

      -> connecting to another computer with NioDatagramConnector.

      for me it looks like in the newHandle method of NioDatagramConnector is not cared about TrafficClass (like it is done in NioDatagramAcceptor.open())

      The server part with the accceptor is OK and the correct ToS byte is set in the packet.
      (the same problem may be in the socket, i have to check it)

        Activity

        Hide
        trustin Trustin Lee added a comment -

        It should be fixed now. Please let me know if it still doesn't work as you expected.

        Show
        trustin Trustin Lee added a comment - It should be fixed now. Please let me know if it still doesn't work as you expected.
        Hide
        martinko martin krivosik added a comment -

        thx very much for quick fix,
        unfortunately it takes me more time to go back to this problem and verify why it does not work correctly.

        in the DefaultDatagramSessionConfig
        are variables
        GET_TRAFFIC_CLASS_AVAILABLE
        SET_TRAFFIC_CLASS_AVAILABLE
        both of them are false, but only the GET_XXXX is setup according the current conditions.

        the SET_XXX is never touched and therefore is not possible to set the traffic mask
        in setTrafficClass() method in NioDatagramSessionConfig class since it is always false..

        Since I do not understand completely the aim if your design and architecture of the lib,
        I will ask you kindly to look on it and provide some fix...

        Show
        martinko martin krivosik added a comment - thx very much for quick fix, unfortunately it takes me more time to go back to this problem and verify why it does not work correctly. in the DefaultDatagramSessionConfig are variables GET_TRAFFIC_CLASS_AVAILABLE SET_TRAFFIC_CLASS_AVAILABLE both of them are false, but only the GET_XXXX is setup according the current conditions. the SET_XXX is never touched and therefore is not possible to set the traffic mask in setTrafficClass() method in NioDatagramSessionConfig class since it is always false.. Since I do not understand completely the aim if your design and architecture of the lib, I will ask you kindly to look on it and provide some fix...
        Hide
        elecharny Emmanuel Lecharny added a comment -

        I think that all those checks (can we set/get traffic class on a socket/datagram) is totally useless. Either you can, or you can't, but in the second case, it simply does nothing.

        Now, the current implementation is totally FU, because we do things like :

        public void setTrafficClass(int trafficClass) {
        if (DefaultDatagramSessionConfig.isSetTrafficClassAvailable()) {
        try {
        c.socket().setTrafficClass(trafficClass);
        ...

        with :
        public static boolean isSetTrafficClassAvailable()

        { return SET_TRAFFIC_CLASS_AVAILABLE; }

        and, ultimately :

        private static final boolean SET_TRAFFIC_CLASS_AVAILABLE = false;
        ^^^^
        This is a dead end : the isSetTrafficClassAvailable() will always return false.

        Here is what I suggest : we simply get rid of all those controls, and let the user set/get the traffic class at will. If the underlaying network does not support it, well, it's fine, no harm.

        Show
        elecharny Emmanuel Lecharny added a comment - I think that all those checks (can we set/get traffic class on a socket/datagram) is totally useless. Either you can, or you can't, but in the second case, it simply does nothing. Now, the current implementation is totally FU, because we do things like : public void setTrafficClass(int trafficClass) { if (DefaultDatagramSessionConfig.isSetTrafficClassAvailable()) { try { c.socket().setTrafficClass(trafficClass); ... with : public static boolean isSetTrafficClassAvailable() { return SET_TRAFFIC_CLASS_AVAILABLE; } and, ultimately : private static final boolean SET_TRAFFIC_CLASS_AVAILABLE = false; ^^^^ This is a dead end : the isSetTrafficClassAvailable() will always return false. Here is what I suggest : we simply get rid of all those controls, and let the user set/get the traffic class at will. If the underlaying network does not support it, well, it's fine, no harm.
        Hide
        elecharny Emmanuel Lecharny added a comment -

        What we can do is something like :

        boolean isSetTrafficClassAvailable(Socket socket) {
        try

        { int tc = socket.getTrafficClass(); socket.setTrafficClass((~tc)&0x001E); boolean supported = (tc != socket.getTrafficClass()); socket.setTrafficClass(tc); return supported; }

        catch (Exception e)

        { return false; }

        }

        but it seems a bit overkilling...

        Show
        elecharny Emmanuel Lecharny added a comment - What we can do is something like : boolean isSetTrafficClassAvailable(Socket socket) { try { int tc = socket.getTrafficClass(); socket.setTrafficClass((~tc)&0x001E); boolean supported = (tc != socket.getTrafficClass()); socket.setTrafficClass(tc); return supported; } catch (Exception e) { return false; } } but it seems a bit overkilling...
        Hide
        ted_kods Edouard De Oliveira added a comment -

        what about this one ?

        boolean setTrafficClassIfAvailable(Socket socket) {
        try

        { int tc = socket.getTrafficClass(); socket.setTrafficClass((~tc)&0x001E); return (tc != socket.getTrafficClass()); }

        catch (Exception e)

        { return false; }


        }

        does the job and informs the user if not done + no unnecessary tests like the previous one which would have lead to something like

        if (isSetTrafficClassAvailable(mysocket)) {
        // set traffic class
        ....;
        }

        Show
        ted_kods Edouard De Oliveira added a comment - what about this one ? boolean setTrafficClassIfAvailable(Socket socket) { try { int tc = socket.getTrafficClass(); socket.setTrafficClass((~tc)&0x001E); return (tc != socket.getTrafficClass()); } catch (Exception e) { return false; } } does the job and informs the user if not done + no unnecessary tests like the previous one which would have lead to something like if (isSetTrafficClassAvailable(mysocket)) { // set traffic class ....; }
        Hide
        elecharny Emmanuel Lecharny added a comment -

        Well, itwon't work :

        • first, you don't pass a new traffic class
        • second, I don't think that it's necessary anyway.

        The contract is pretty clear :
        "As the underlying network implementation may ignore this value applications should consider it a hint. "

        http://java.sun.com/j2se/1.4.2/docs/api/java/net/Socket.html#setTrafficClass(int)

        Let's do simple things, and when it becomes complicated, just think twice before injected convoluted code.

        Show
        elecharny Emmanuel Lecharny added a comment - Well, itwon't work : first, you don't pass a new traffic class second, I don't think that it's necessary anyway. The contract is pretty clear : "As the underlying network implementation may ignore this value applications should consider it a hint. " http://java.sun.com/j2se/1.4.2/docs/api/java/net/Socket.html#setTrafficClass(int ) Let's do simple things, and when it becomes complicated, just think twice before injected convoluted code.
        Hide
        ted_kods Edouard De Oliveira added a comment -

        indeed, the example was just intended to highlight the unnecesseray redundant tests happening with the use of a isSetTrafficClassAvailable(...) method.

        http://java.sun.com/j2se/1.4.2/docs/api/java/net/Socket.html#getTrafficClass()
        also states that "the underlying network implementation may ignore the traffic class or type-of-service set using #setTrafficClass()"
        If any error throws an exception i just wonder why the hell this code got so complicated ?

        ps : tried to search through mina jira db but found nothing proving that it could fail silently

        Show
        ted_kods Edouard De Oliveira added a comment - indeed, the example was just intended to highlight the unnecesseray redundant tests happening with the use of a isSetTrafficClassAvailable(...) method. http://java.sun.com/j2se/1.4.2/docs/api/java/net/Socket.html#getTrafficClass( ) also states that "the underlying network implementation may ignore the traffic class or type-of-service set using #setTrafficClass()" If any error throws an exception i just wonder why the hell this code got so complicated ? ps : tried to search through mina jira db but found nothing proving that it could fail silently
        Hide
        elecharny Emmanuel Lecharny added a comment -

        The question is : do we really need this method in 2.0 (ie, isSetTrafficClassAvailable). The main problem is that we need an open socket in order to check that, and we have to set the traffic class, and reset it, to be sure that it's possible. As the Traffic Class is absolutely not guaranteed to be used by the underlying network layer, I think it's a lot of effort for a small improvement.

        Now, this can be discussed. I understand that, from the user POV, masking all this complexity is good, but when a user tries to manipulate such advanced features, I think that it should be explicit.

        Show
        elecharny Emmanuel Lecharny added a comment - The question is : do we really need this method in 2.0 (ie, isSetTrafficClassAvailable). The main problem is that we need an open socket in order to check that, and we have to set the traffic class, and reset it, to be sure that it's possible. As the Traffic Class is absolutely not guaranteed to be used by the underlying network layer, I think it's a lot of effort for a small improvement. Now, this can be discussed. I understand that, from the user POV, masking all this complexity is good, but when a user tries to manipulate such advanced features, I think that it should be explicit.
        Hide
        vrm Julien Vermillard added a comment -

        Agree with Emmanuel, so much glue and troubles for masking few of the complexity of a such advanced feature.

        Show
        vrm Julien Vermillard added a comment - Agree with Emmanuel, so much glue and troubles for masking few of the complexity of a such advanced feature.
        Hide
        elecharny Emmanuel Lecharny added a comment -

        Postponed to 2.0.1

        Show
        elecharny Emmanuel Lecharny added a comment - Postponed to 2.0.1
        Hide
        elecharny Emmanuel Lecharny added a comment -

        Too complex to implement in 2.0.

        To be discussed in 3.0

        Show
        elecharny Emmanuel Lecharny added a comment - Too complex to implement in 2.0. To be discussed in 3.0

          People

          • Assignee:
            elecharny Emmanuel Lecharny
            Reporter:
            martinko martin krivosik
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 20m
              20m
              Remaining:
              Remaining Estimate - 20m
              20m
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development