0

I have a Java Servlet that has a repeated code I would put it out into a method to clean the code.

The problem is I have read that could be concurrency problems if I use auxiliary methods.

Is that true?

This is my code, which is repeated several times:

if(session.getAttribute("attemps") == null) {
   session.setAttribute("attemps", 1);
}
else {
   Integer attemps = (Integer) session.getAttribute("attemps");
   session.setAttribute("attemps", attemps + 1);
}

I would create a method with this code inside and then call the method when needed.

Thanks.

Ommadawn
  • 2,450
  • 3
  • 24
  • 48
  • Is the concurrency going to happen in same session? Or are you talking about Servlet being called by multiple requests? – Nick Div Jun 24 '16 at 19:19
  • I was talking about the Servlet being called by multiple requests. That's my main preoccupation, but, if you have the solution for both... I'll be glad :) – Ommadawn Jun 24 '16 at 19:23
  • 1
    For Servlet being called by multiple requests each session is going to have its own attributes and consolidating the code should be fine. Unless with in each request you are doing some multi-threading within. If you are worried about concurrency with in same request I suggest you read up on Synchronized methods and allow only one thread to access the method at a time. – Nick Div Jun 24 '16 at 19:42
  • Thank you, @NickDiv :) That was helpful! – Ommadawn Jun 24 '16 at 19:58

1 Answers1

1

Concurrency problems don't happen when you extract code to a separate method. Concurrency problems happen when multiple concurrent threads access mutable state.

You do have mutable state here: the session. Two requests coming from the same browser can be handled concurrently, and that can lead to a race condition. The session itself is thread-safe, but since you're doing check-then-act and get-then-set actions on the session, although the session is safe, you could just end up with an incorrect result.

For example, two threads might concurrently check for null, could concurrently see the attempts attribute as null, and could both set its value to 1. You would thus end up with 1 attempt instead of 2.

JB Nizet
  • 678,734
  • 91
  • 1,224
  • 1,255
  • Thanks, @JBNizet ! So you think is a good idea to wrap the content of my method and do something like syncronized(lock){ //my content } – Ommadawn Jun 24 '16 at 22:02
  • If the method is in a servlet, which is a singleton, then yes. I would rather use a dedicated fstatic final lock, though. Note that both strategies will block other threads from other sessions to execute, though. Probably not a bug deal. – JB Nizet Jun 24 '16 at 22:11