Review Board 1.5.4

Issue 436 No shut-down during comet

Updated 1 year, 10 months ago

David Pollak Reviewers
dpp, marius, charles, heiko, joni, atsuhiko, jorge, naftoli, kris, indrajit, alexb, dlouwers, mstarzyk, rmellgren, jhoffman, probinett, jmadsen, mhartmann, jstrachan, jgoday
None LiftWeb-archive 
https://liftweb.assembla.com/spaces/liftweb/tickets/436-comet-doesn-t-release-during-shutdown

dpp_issue_436

Somehow the code that shuts down sessions when the Filter is unloading got lost.  I added the session shutdown code and did some cleanup of various shutdown code.

I also added a line to not cache resource change dates in dev mode
Manual testing of comet-based apps include examples/example and making sure shutdown was correct and quick.
Marius Danciu
Posted 1 year, 10 months ago (March 24th, 2010, 11:34 a.m.)

   

  

   
    
    
   


  




   



    

     
     
    

    

     
is this implicit really necessary? I understand the purpose but I'd prefer dealing with () => Box[LiftResponse] a little more explicitly

     

  

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Okay.  I'll make it non-implicit
    
    
    
   
    2. 

   


  



    

    

    

     
     
    

    

     
This looks a little awkward ... it is not the same due to implicit but the code is not very clean IMHO

     

  

   
    
    
   


  



    

    

    

     
     
    

    

     
Isn't S already initted here?

     

  

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    No.  The whole point of passing the function back is to not be in S.init while waiting for the long poll.  Inside the S.init block, we're consuming a DB connection and potentially other "S.addAround" resources.  The code moves the long part of the long polling out of an S.init block, but makes sure we're in the S.init block during the answer conversion.
    
    
    
   
    2. 

   


  



    

    

   


  
   
Other things are looking good to me. I'll wait for your feedback before clicking "Ship it"

   

  

   
    
    
   


  



  
 




  

 














 

  




 

   Review request changed
   
Updated 1 year, 10 months ago (March 24th, 2010, 11:58 a.m.)

 

 

  

  
  
Made the conversion not implicit


 




  

 













 

  




 

 

  
  
Ross Mellgren

  
Posted 1 year, 10 months ago (March 24th, 2010, 12:01 p.m.)

 

 

   
were the changes made to LiftRules in the most recent diff intentional? they look like they may have leaked from another change

   

  

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Yes.  It's part of the shutdown logic... it lets requests bleed off until there are no requests to service before doing shutdown logic.
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    Oh I don't mean the changes that were part of the original diff you uploaded, I mean the additional changes to LiftRules added in the second diff you uploaded with the note "made the conversion not implicit" -- they refer to dev mode and dynamic reload of the sitemap, which I think is for the other feature you were working on.

    
    
    
   
    3. 

   


  




  
 




  

 


















 

  




 

 

  
Ship it!

  
Marius Danciu

  
Posted 1 year, 10 months ago (March 24th, 2010, 1:14 p.m.)

 

 

   
Looks good to me.