aspose file tools*
The moose likes Servlets and the fly likes Counter using Session Tracking !! Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login
JavaRanch » Java Forums » Java » Servlets
Bookmark "Counter using Session Tracking !!" Watch "Counter using Session Tracking !!" New topic
Author

Counter using Session Tracking !!

Vaibhav Shridish
Greenhorn

Joined: Jun 06, 2002
Posts: 28
This servlet is being accessed from a html page which has a submit button.
In reply this servlet has to display the count of
the number of times the servlet has been accessed.
==============================================
can some one please let me know y the following code is not displaying the count ??
===============================================
import java.io.*;
import javax.servlet.*;
import javax.servlet.http.*;
import java.util.*;
public class CounterServlet extends HttpServlet
{
Integer count;
public void doGet(HttpServletRequest req, HttpServletResponse resp)
throws ServletException, IOException
{
resp.setContentType("text/html");
PrintWriter out=resp.getWriter();
HttpSession hs =req.getSession(true);
out.println("<HTML>");
out.println("<HEAD>");
out.println("<TITLE>CounterServlet</TITLE>");
out.println("<BODY>");
out.println(hs.isNew());
if(hs.isNew())
{
hs.setAttribute("counter",new Integer(0));
}
count=(Integer) hs.getAttribute("counter");

synchronized(count)
{
count=new Integer(count.intValue()+1);
out.println("Count value is -"+count.intValue());
}
out.println("<center>");
out.println("<B>The counter Servlet has been accessed "+ count.intValue() +" times");
out.println("</center>");
out.println("</BODY>");
out.println("</HEAD>");
out.println("</HTML>");
out.close();
}
}


Vaibhav Shridish
David Hibbs
Ranch Hand

Joined: Dec 19, 2002
Posts: 374
Well, there are several issues here that could cause you problems.
First, you're trusting your servlet engine's session isNew method. While probably OK, this will reset for every user. If you want the total hit count, this won't work. If you want the hit count for this user, this should work OK as long as the user doesn't close the browser or otherwise dispose of their session cookie. You're better off trying to retrieve the value though and checking if it's null.
Next, you're trying to synchronize on an instance variable. This instance variable can be changed by each user's thread. So if 5 users hit it, and refresh several times, you'll get mixed and erroneous results. Get rid of the instance variable.
Third, you never update the integer value in the session. So the first time through, you put zero in the session. The next time through (and every other time through), count is taken from the session, and is zero.
Fourth, you probably don't want to generate HTML in your Servlet. This is better left for JSP's. But if you're just learning servlets, this is probably OK for now as long as you plan on learning JSP's soon. =)


"Write beautiful code; then profile that beautiful code and make little bits of it uglier but faster." --The JavaPerformanceTuning.com team, Newsletter 039.
Mike Curwen
Ranch Hand

Joined: Feb 20, 2001
Posts: 3695

I'd agree with David on all counts except one. I don't think you necessarily need to get rid of the instance variable, only use it better.

Can anyone see anything wrong with this code:I admit, you probably don't even need the call to synchronize (this), but for the absolute safety it guarantees about the ++counter and the assignment to not be interupted, it is included.

The other thing I'd add, is that I'm not sure why you are doing anything at all with the user's session. The number of times a particular servlet has been hit has nothing to do with any single user. In addition, the count retained in someone's session would be out of date the instance someone else hit the servlet.

If you *need* to put the count in the session, simply put the contents of currentCount in there. Don't bother checking if the session is new either.
[ February 24, 2003: Message edited by: Mike Curwen ]
Jim Yingst
Wanderer
Sheriff

Joined: Jan 30, 2000
Posts: 18671
Can anyone see anything wrong with this code:
Looks good to me. However I'm a little confused - you said you disagree with David Hibbs, that it's not necessary to get rid of synchronizing on the instance variable. But then you proceed to change the synchronization to sync on the Thread instance (this) rather than the instance variable (count). I think David was right, and so is your code - it's better to sync on the Thread instance. In the previous code, Vaibhav wasn't really syncing on the variable count - he was suncing on the Integer object which it referenced. The variable was getting changed all the time, making it refer to different Integer objects - which means that two different threads trying to run the same code could be syncing on different objects, meaning that they could both acquire locks simultanously and thus two threads could be running the same "protected" code at the same time. Not good. In practice I doubt this would cause much real trouble, but it's risky. If you're going to sync on anything, sync on the Tread instance.
I admit, you probably don't even need the call to synchronize (this),
Probably true unless it's important that the count be exactly accurate. It may be better to declare count as volatile instead, and drop the synchronization. This allows faster access, while still requiring the variable to be properly updated every time it's accessed. (Otherwise two different threads may be incrementing their own local copies of the variable, only updating the instance variable later - and one thread's update will overwrite the changes made by the other thread.
Even with volatile, there's a minor problem. The ++ operation is not atomic. In order to perform an increment, the JVM must read the value of count from memory, increment it, and store the result back in memory. In between these steps, it's possible for another thread to interrupt (again, even with volatile). If this happens, your count will end up one less than it should be. This will usually be pretty rare, and most of the time no one really cares if the count was 4378 or 4377. So it may be worthwhile to get a slight speed boost using volatile rather than synchronized, at the cost of a little accuracy.
Note also that if count were long (or double), then forget about using volatile. Too many JVMs do not handle long or double values safely (in violation of the VM Spec). If you want to work with these datatypes safely, you must use synchronization. Worth remembering if you ever have a really popular website, requiring a long for the hit count.


"I'm not back." - Bill Harding, Twister
Mike Curwen
Ranch Hand

Joined: Feb 20, 2001
Posts: 3695

Thanks for your comments Jim, and no, I said it wasn't necessary to get rid of the instance variable (not the synchronizing).
David Hibbs
Ranch Hand

Joined: Dec 19, 2002
Posts: 374
Originally posted by Mike Curwen:
Can anyone see anything wrong with this code:

Yep. The servlet engine can, at its own perrogative, create multiple instances of the same servlet. For example, if it gets 5 requests for the same servlet at once, it's allowed to create two instances of the servlet and split the requests between them. So instead of the last user seeing 5 hits, he/she may see any number from 1 to 5.
Of course, other issue arises if the app or server gets restarted... the counter starts over at zero. But I think we were all aware of that little limitation.
Jim Yingst
Wanderer
Sheriff

Joined: Jan 30, 2000
Posts: 18671
The servlet engine can, at its own perrogative, create multiple instances of the same servlet.
Here's where my limited knowledge of servlets may bite me. I understood that there would only be one servlet, but that there may be multiple threads (unless you're using the single-thread model). That's what Marty says on p. 48 of "More Servlets". Not true?
If there may be multiple instances of CounterServlet, then Mike's code can be fixed by making counter static, and replacing
synchronized (this) {
with
synchronized (CounterServlet.class) {
OK?
Mike Curwen
Ranch Hand

Joined: Feb 20, 2001
Posts: 3695

From the 2.4 servlet spec... (and this has not changed since 2.3)

I dislike the SingleThreadModel, so didn't consider it when I wrote my code. I assumed single-container (and single-instance) of the servlet that is multi-threaded.
[ February 25, 2003: Message edited by: Mike Curwen ]
 
Don't get me started about those stupid light bulbs.
 
subject: Counter using Session Tracking !!