11 Replies Latest reply on Sep 22, 2006 1:43 PM by Ovidiu Feodorov

    JBMESSAGING-549 - Lease set to -1 causes exception

    Clebert Suconic Master

      Look at ClientCrashNegativeLeaseTest before reading this post:


      The problem is we are setting Lease to -1 after we passed Lase=20000 on the properties of the invoker.

      Lease set to -1 is not setting leaseManagement to false (because of the prior set).


      So, a fix would be on remoting:

      ///
       public void setLeasePeriod(long leasePeriodValue)
       {
       this.leasePeriod = leasePeriodValue;
       if (leasePeriod<0)
       {
       this.leaseManagement=false;
       }
       }
      





        • 1. Re: JBMESSAGING-549 - Lease set to -1 causes exception
          Clebert Suconic Master

          I have commited the proposed fix on Remoting.
          If this is not okay, please revert it (after finding another solution of course :-) )

          • 3. Re: JBMESSAGING-549 - Lease set to -1 causes exception
            Tom Elrod Master

            I don't really understand what you are trying to achieve, so if can explain that better, can help with how to do it (i.e. stop future clients from establishing lease sessions, clean up current server side lease sessions, etc.).

            Currently the only way to disable leasing from the server side (meaning will tell any future clients doing initial lease query that server leasing is disabled AND clean up any current lease sessions in progress on the server side) is to remove all ConnectionListeners on the server side. Setting the lease period to negative value after lease management has been enabled, does not have any impact other then if remove all ConnectionListeners and then add a new ConnectionListener, leasing will NOT be enabled (since the lease period is negative). Admittedly, this is not well documented within remoting guide (I will update the doc).

            The change for JBREM-602 does prevent future clients doing a lease query from starting a lease session, but does not clean up an current lease sessions in progress.

            Should also mention that there is currently nothing within the code to prevent clients from sending lease ping once leasing has been disabled on the server side. They will just continue to send lease ping, which will be thrown away on the server side.

            So what I need to understand is exactly what it you need (requirements) and will make it happen.


            • 4. Re: JBMESSAGING-549 - Lease set to -1 causes exception
              Clebert Suconic Master

              The problem is...

              if you set Lease (on Server) to -1, after its creation...
              whenever you receive a $PING$ you get an exception.



              At this point I just want to understand semantics of disabling lease on Serverinvoker. Maybe setLeasePeriod should be deprecated/removed if this is the correct semantic.

              • 5. Re: JBMESSAGING-549 - Lease set to -1 causes exception
                Tom Elrod Master

                You are right that client should not error because the server says its default lease period is a negative number. I have already checked in a fix for this.

                However, we need to resolve how all of this works together for leasing. First off, if there is no connection listener registered on the server side, there is no point in doing leasing at all imo (as there will be no one to notify of expired lease). This is why lease period > 0 AND at least one connection listener registred is required to turn on leasing from the server side.

                Once leasing is turned on and a client has begun to do leasing (meaning there is a leasing sessions in progress on the server side), need to know when they should be cleaned up. Currently there are three cases when this will happen:

                1. Client disconnects
                2. Client lease is expired (meaning client probably crashed)
                3. All connection listeners are removed

                Notice case 3 is because leasing has been disabled.

                Then there is the leasing period on the server side. This value is only used as part of the criteria for turning on leasing (having connection listener registered is the other). Once leasing is enabled on the server side and leasing sessions have begun, the lease period on the server side only impacts what the leasing period will be on the client (assuming the client does not override this with their own lease period). This is where the original bug mentioned came in because the client would accept the server's lease period without checking for negative number.

                So having said all that, does this NOT meet messaging requirements? If not, what should be changed?

                • 6. Re: JBMESSAGING-549 - Lease set to -1 causes exception
                  Clebert Suconic Master

                  I guess when ServerInvoker::setLeasePeriod is set to -1, we shouldn't change any previous estabilished Leases. That would change the behavior of futurue clients...

                  and I guess this is how it's behaving now (if checking the property during setLeasePeriod)

                  BTW when we call LeasePeriod to -1, we didn't have any Lease estabilished yet.

                  • 7. Re: JBMESSAGING-549 - Lease set to -1 causes exception
                    Tom Elrod Master

                    To make things a little simplier, do you think it would be useful to add another method such as:

                    void setLeaseEnabled(boolean)

                    If call setLeaseEnabled(boolean) passed false and are current lease session, will clean them up. If passed true, will accept lease sessions even if no connection listeners (will just have no one to notify).

                    Also, if try to call setLeasePeriod(long period) with number < 0, will throw an exception.

                    Although a little more blunt in how things are handled, is much simplier from user API perspective.

                    • 8. Re: JBMESSAGING-549 - Lease set to -1 causes exception
                      Clebert Suconic Master

                       

                      To make things a little simplier, do you think it would be useful to add another method such as:

                      void setLeaseEnabled(boolean)


                      It would make sense IMO.

                      However if you throw an exception on setLeasePeriod(<0) isn't that a signature change? We would have to change some testcases using setLeasePeriod(-1) hoping to disable lease.

                      If you *must* change the API we will need to plan ahead.

                      • 9. Re: JBMESSAGING-549 - Lease set to -1 causes exception
                        Tom Elrod Master

                        Well, don't have to change anything, just seemed like would help avoid confusion in the future as to how everything works. If think adding setLeaseEnabled(boolean) would be better for you guys then would require you guys to change your code to call setLeaseEnabled(false) instead of using setLeasePeriod(-1) to disable leasing. Is really your call as to if would be worth making the code change (in both remoting and messaging) to gain this more explicit way of enabling/disabling leasing.

                        My guess (and again is just my outside perspective) is that you would not want to introduce an api change?

                        • 10. Re: JBMESSAGING-549 - Lease set to -1 causes exception
                          Clebert Suconic Master

                          I guess we can change the API.. since thie method is on a testcase and it's kind of non public API. (What you think Ovidiu? Tim?)

                          I just think we should make a plan on change this.
                          I just want to avoid having exceptions due to this change when you have the next release. We would have to change it right after you released.

                          • 11. Re: JBMESSAGING-549 - Lease set to -1 causes exception
                            Ovidiu Feodorov Master

                            How about we don't change anything? Let's keep it simple.
                            A setLeasePeriod(-1) seems clear enough and good enough for me.