This discussion is archived
12 Replies Latest reply: Feb 9, 2012 10:12 PM by r035198x RSS

Thread safety check

802569 Newbie
Currently Being Moderated
Hi, I need some confirmation that this design is thread safe.

So I have a servlet with one instance variable used for authentication (I'll refer to the instance as auth). This authentication class has only one instance variable used for common jdbc functions (we'll call it jdbc) and this jdbc class does not have any instance variables.

So, when a request is posted to the servlet, two params are retrieved from the request and passed to a method in auth; auth will use its jdbc variable to grab a connection from a pool via jndi lookup; once auth has a connection it uses it to authenticate against a database. See basic code below.

I want to ensure that auth and jdbc are thread safe and can handle multiple requests. My concern is not about the request variables passed to auth, but more so with the connection which is returned from the lookup. Also, would the thread safety change if the jdbc methods were all static? Thanks for the help.
public class MyServlet extends HttpServlet {

        private Authentication auth = new Authentication();

     protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
          String username = req.getParameter("user");
          String password = req.getParameter("pass");
          
          User user = auth.authenticate(username, password);
             ...
          
     }
        ...
}



public class Authentication {
     
     private JdbcUtil jdbc = new JdbcUtil();

     public User authenticate(String username, String password) {

          Connection con = null;
          try {
               con = jdbc.getCon();
                        ....

       }
}


public class JdbcUtil {

     public Connection getCon() throws SQLException, NamingException {

          Context ctx = new InitialContext();
          DataSource ds = (DataSource) ctx.lookup("java:comp/env/jdbc/MyDB");
          return ds.getConnection();

     }
        ....
}
  • 1. Re: Thread safety check
    796440 Guru
    Currently Being Moderated
    799566 wrote:
    Hi, I need some confirmation that this design is thread safe.

    So I have a servlet with one instance variable
    Servlets should never have any instance variables. There can be multiple requests being serviced simultaneously by a single Servlet instance.
  • 2. Re: Thread safety check
    DrClap Expert
    Currently Being Moderated
    Apart from thread-safety, another problem with that design is that you take a connection from the connection pool upon the first request for the servlet and you never return that connection to the pool. Hanging on to connections is a bad practice in a web application.
  • 3. Re: Thread safety check
    802569 Newbie
    Currently Being Moderated
    jverd wrote:
    Servlets should never have any instance variables. There can be multiple requests being serviced simultaneously by a single Servlet instance.
    But since the request variables are local variables (and subsequently so are the parameters passed to the authenticate method), then they would be part of that threads own stack, not a common one, correct?? If not, the alternative is synchronize the methods or create a new instance for each request?


    p.s. I didn't post all the code, the connection is dealt with properly after it serves its purpose.
  • 4. Re: Thread safety check
    DrClap Expert
    Currently Being Moderated
    799566 wrote:
    p.s. I didn't post all the code, the connection is dealt with properly after it serves its purpose.
    In which case what you should do is to create a new Authentication object for each request. Apparently it doesn't store any permanent information (unless that's in the code you didn't post) so there's no reason for it to be a permanent attribute of the servlet. That way the question about thread-safety becomes moot.
  • 5. Re: Thread safety check
    796440 Guru
    Currently Being Moderated
    799566 wrote:
    jverd wrote:
    Servlets should never have any instance variables. There can be multiple requests being serviced simultaneously by a single Servlet instance.
    But since the request variables are local variables (and subsequently so are the parameters passed to the authenticate method), then they would be part of that threads own stack, not a common one, correct??
    That's correct. But what does that have to do with instance variables? In your first post you said "I have a servlet with one instance variable." That is an incorrect design, since one call to the service(), doPost(), etc. method might see a value that was set for a different call, instead of its own value.
    If not, the alternative is synchronize the methods or create a new instance for each request?
    If you want different state, you create a new instance. That's how OO works, and it has nothing to do with servlets in particular. But we don't control the creation of Servlets, so we ned a different object if we want to save state. A first cut simple approach would be to define some MyRequestState class that has member variables for everything you care about as state during one call to doPost().
    protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
      MyRequestState state = new MyRequestState();
      // Now we initialize the MRS object with whatever values are relevant from the request, session, context, whatever.
      // Then we go on to use it however we need.
    
      doSomething(state);
      doSomethingElse(state);
    }
    Another approach is that instead of just a state holder object, we define a class that holds the relevant state we need AND has a single execute() method
    protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
      MyRequestHandler handler = new MyRequestHandler();
      handler.setXxx(...);
      handler.setYyy(...);
      handler.execute();
    }
  • 6. Re: Thread safety check
    802569 Newbie
    Currently Being Moderated
    jverd wrote:
    That's correct. But what does that have to do with instance variables?
    Well, if I can avoid having to create a new Authentication object for each new thread, then wouldn't that be a good reason to have an instance variable. I know the instance variable will have multiple threads using it, but I'm more concerned with whether the threads will step on each others toes with respect to the request data and authentication process (at the very least to support my own understanding of potential multithreading issues for this scenario).
  • 7. Re: Thread safety check
    gimbal2 Guru
    Currently Being Moderated
    799566 wrote:
    jverd wrote:
    That's correct. But what does that have to do with instance variables?
    Well, if I can avoid having to create a new Authentication object for each new thread, then wouldn't that be a good reason to have an instance variable.
    Why do you want to avoid that? I am guessing you have the premature optimization disease.
  • 8. Re: Thread safety check
    796440 Guru
    Currently Being Moderated
    799566 wrote:
    jverd wrote:
    That's correct. But what does that have to do with instance variables?
    Well, if I can avoid having to create a new Authentication object for each new thread, then wouldn't that be a good reason to have an instance variable.
    Don't be afraid of creating objects. They're a natural part of OO. And, again, you switched from talking about instance variables to local variables, so I was asking what those local variables have to do with the instance variables you started off with.
    I know the instance variable will have multiple threads using it, but I'm more concerned with whether the threads will step on each others toes with respect to the request data and authentication process (at the very least to support my own understanding of potential multithreading issues for this scenario).
    No idea what you're saying here, so let's get back to basics: A Servlet should never have an instance variable. Ever. Period. End of story. I've already explained why, but if there's something about that explanation you don't get, please ask.
  • 9. Re: Thread safety check
    802569 Newbie
    Currently Being Moderated
    Ok. I will never create a servlet instance variable again:)

    But for the sake of understanding concurrency, is what I've presented above safe. In other words, if two people login at the time, is there any risk of the authentication process for one user interfering with that of the other (e.g. can the username/pwd's be overwritten, will each thread get its own db connection, etc.)?
  • 10. Re: Thread safety check
    gimbal2 Guru
    Currently Being Moderated
    799566 wrote:
    But for the sake of understanding concurrency, is what I've presented above safe. In other words, if two people login at the time, is there any risk of the authentication process for one user interfering with that of the other (e.g. can the username/pwd's be overwritten, will each thread get its own db connection, etc.)?
    Like this whole thread has proven, that depends on your code. If you write the code such that two threads can use the same exact object at the exact same time, things will likely go boom unless you synchronize access. But if you write proper dumb code; making sure that each thread will create its own private objects and will use its own private transaction, you're quite safe.
  • 11. Re: Thread safety check
    796440 Guru
    Currently Being Moderated
    799566 wrote:
    But for the sake of understanding concurrency, is what I've presented above safe.
    Not even gonna look at the code, since it's a servlet, but if it's a servlet and it has an instance variable, then, no, it's not, unless you synchronized all access to that variable, which is a bad thing to do in a servlet.

    If you want to play with concurrency and thread-safety, do it outside of a servlet context, so that you can produce [url http://sscce]SCCEs to easily test your hypotheses and demonstrate your questions here.
  • 12. Re: Thread safety check
    r035198x Pro
    Currently Being Moderated
    Take a step back and look at your code.
    How many instances of the Authentication object does your application need?
    Now look at your Authentication class. You are doing it again. How much does it cost to create a new JdbcUtil object?
    How many instances of JdbcUtil does your application need?
    Why do I need to create an instance of JdbcUtil to be able to call getCon()?
    What's stopping you from doing
    DataSource ds = (DataSource) ctx.lookup("java:comp/env/jdbc/MyDB");
    in the doPost method itself?
    Also have a read around the internets on how others have solved this problem (you'll be surprised at how many people have needed to do the same thing before) .

Legend

  • Correct Answers - 10 points
  • Helpful Answers - 5 points