testRegisterPublisher and testRegisterPublisherRetries are broken #268

Activity

1. dambrosio commented on Feb 26, 2018

   

   dambrosio

   on Feb 26, 2018

   More actions

   I will need to get my setup configured on my personal computer and then run the tests. Unfortunately, I did not run the gradle tests after my commit (☹️ ) so I cannot say if the tests failed or passed for me. I will do this today and try and get back with you guys.

   PS. I am using my personal account instead of my work. Just a heads up.
2. drigz commented on Feb 26, 2018

   

   drigz

   on Feb 26, 2018

   MemberAuthor

   More actions

   Thanks! Let us know if you make any progress!
3. dambrosio commented on Feb 26, 2018

   

   dambrosio

   on Feb 26, 2018

   More actions

   @drigz
   I am also failing on the following tests:

   org.ros.node.service.ServiceIntegrationTest > testPesistentServiceConnection FAILED
       java.util.ConcurrentModificationException
   
   org.ros.node.service.ServiceIntegrationTest > testRequestFailure FAILED
       java.util.ConcurrentModificationException
   
   org.ros.internal.node.MasterRegistrationTest > testRegisterPublisher FAILED
       java.lang.AssertionError at MasterRegistrationTest.java:45
   
   org.ros.internal.node.MasterRegistrationTest > testRegisterPublisherRetries FAILED
       java.lang.AssertionError at MasterRegistrationTest.java:74
   

   I'm testing on Ubuntu 16.04, Kinetic, Gradle 2.14.1 and Oracle Java 1.80

   I will start to look into the publisher tests and see if I can see why they are failing...I haven't looked at this code in a while so it might take a bit.
4. dambrosio commented on Feb 27, 2018

   

   dambrosio

   on Feb 27, 2018

   More actions

   @drigz It seems as if the Master is successfully sending the unregisterPublisher xml response by looking at the network traffic. I changed the publisher to /register so I could filter the network traffic.

   Here is the unregisterPublisher request triggered by publisher.shutdown() on line 44 of MasterRegistrationTest.java:

   Transmission Control Protocol, Src Port: 59259, Dst Port: 38705, Seq: 1, Ack: 1, Len: 424
   Hypertext Transfer Protocol
   eXtensible Markup Language
       <?xml
           version="1.0"
           encoding="UTF-8"
           ?>
       <methodCall>
           <methodName>
               unregisterPublisher
               </methodName>
           <params>
               <param>
                   <value>
                       /node
                       </value>
                   </param>
               <param>
                   <value>
                       /register
                       </value>
                   </param>
               <param>
                   <value>
                       http://127.0.0.1:35013/
                       </value>
                   </param>
               </params>
           </methodCall>
   
   

   And here is the response from the Master:

   Transmission Control Protocol, Src Port: 38705, Dst Port: 59259, Seq: 1, Ack: 425, Len: 329
   Hypertext Transfer Protocol
   eXtensible Markup Language
       <?xml
           version="1.0"
           encoding="UTF-8"
           ?>
       <methodResponse>
           <params>
               <param>
                   <value>
                       <array>
                           <data>
                               <value>
                                   <i4>
                                       1
                                       </i4>
                                   </value>
                               <value>
                                   Success
                                   </value>
                               <value>
                                   <i4>
                                       1
                                       </i4>
                                   </value>
                               </data>
                           </array>
                       </value>
                   </param>
               </params>
           </methodResponse>
   

   I think what is happening is the ListenerGroup in the publisher is being shutdown before the master unregister signal is caught and thus causing the timeout. It is almost like there should be a check to see if the signalOnMasterUnregistrationSuccess() function has been executed before the listeners.shutdown() is called.

   File: DefaultPublisher.java

     @Override
     public void shutdown(long timeout, TimeUnit unit) {
       signalOnShutdown(timeout, unit);
       outgoingMessageQueue.shutdown();
       // Add a timer check to see if we got the unregister signal from master, then proceed to shutdown all listeners 
       listeners.shutdown();
     }

   Here is where the unregister gets triggered in the DefaultPublisher.java class

   File: DefaultPublisher.java

   @Override
     public void signalOnMasterUnregistrationSuccess() {
       final Publisher<T> publisher = this;
       listeners.signal(new SignalRunnable<PublisherListener<T>>() {
         @Override
         public void run(PublisherListener<T> listener) {
           listener.onMasterUnregistrationSuccess(publisher);
         }
       });
     }

   Any ideas?
   @jubeira adding you to this for input, let me know if you have questions.
5. jubeira

   added

   Good first issue

   on Mar 15, 2019
6. jubeira

   closed this as completedin #297on Apr 10, 2019