CODEWITHSUNDEEP
jakarta-eeservletsrace-condition
Hardy
Hardy

Reputation: 477

Race condition in a Java Servlet

I have this weird problem where two separate users upload a picture to my web service and user1 see's user2's error messages. Reviewing the code nothing sticks out to me, so I would like to ask what is wrong with this code and why was it creating this condition where errors for user2 would be visible to user1?

Here is some simplified code to try and demonstrate the situation. 

public class SomeService {

    private static SomeService service;
    private SomeService() {
    }

    public static SomeService getInstance() {
        if(service == null) {
           service = new SomeService();
        }
    }

    public ErrorStatus doSomething(ErrorStatus es) {

        es = new ErrorStatus(es);
        // stuff happens that causes an error
        es.addMessage(new ErrorMessage("some error happened"));
        return es;
    }

    public ErrorStatus doSomethingElse(ErrorStatus es) {

        es = new ErrorStatus(es);
        // stuff happens that causes an error
        es.addMessage(new ErrorMessage("some different error happened"));
        return es;
    }
}

public class ErrorMessage {
    String message;

    //simple constructor, getters and setters, nothing interesting
}

public class ErrorStatus {
    int id;
    String status;
    List<ErrorMessage> messages;

    public ErrorStatus() {
         id = 0;
         status = "";
         messages = new ArrayList<>();
    }

    public ErrorStatus(ErrorStatus other) {
        id = other.getId();
        status = other.getStatus();
        messages = other.getMessages();
    }

    public void addMessage(ErrorMessage message) {

        //data checks
        messages.add(message);
    }

    //getters and setters
}   

public class UploadServlet extends HttpServlet {

    public doGet(request, response) {

        ErrorStatus es = new ErrorStatus();
        SomeService service = SomeService.getInstance();

        es = service.doSomething(es);

        es = service.doSomethingElse(es);

        printErrors(response.getWriter(), es);

    }

    public void printErrors(PrintWriter pw, ErrorStatus es) {

        for(int i = 0; i < es.getMessages().size(); i++) {

            pw.write(es.getMessages().get(i).getMessage());
        }
    }
}

The two places in the code I think something weird could be happening is the copy constructor or the fact that the service is a singleton. Maybe I'm not copying the list correctly, or maybe the service being a singleton changes how the stack and heap are used, I'm really not sure. From my understanding of how the stack, heap, singletons and servlets work one users data would never be affected by another users data. Also they only part of this that ever had problems was the list, the primitive data was always correct, only the list of errors was ever showing up for the wrong user.

I should note, I have solved the problem I just don't understand why it was a problem. The solution was to stop using the copy constructor and to just let the ErrorStatus object get modified in the doSomething and doSomethingElse methods. So the revised code would have a void return type for doSomething and would just call es.addMessage, also the copy constructor was deleted from ErrorStatus.

Any help understanding why this caused a race condition would be appreciated.

Upvotes: 0

Views: 4633

Answers (2)

kchang
kchang

Reputation: 91

I agree with one of the responses in that, it's hard to figure out the problem from your simplified code. Some typos/errors I noticed:

public static SomeService getInstance() {
    if(service == null) {
       service = new SomeService();
    }
    **return service;**
}

public doGet(request, response) {

    ErrorStatus es = new ErrorStatus();
    SomeService service = SomeService.getInstance();

    es = service.doSomething(es); //<-- This first one never gets tracked; es is overwritten below.

    es = service.doSomethingElse(es); 

    printErrors(response.getWriter(), es);

}

public ErrorStatus() {
     id = 0; **<-- How exactly does id increase?**
     status = ""; 
     messages = new ArrayList<>();
}

In any case, check if you have member variables in either SomeService or UploadServlet. I am learning about race conditions myself; I suspect you have a problem where two users are accessing a shared resource, and that the interleaving/timing of their access is the problem.

For a simple example, a user might: get servlet instance (gsi) --> do actions (da) --> log errors (le) --> read errors (re).

Two users may interleave operations.

User1: gsi --> da --> le --> re

User2: __ gsi --> da --> le --> re.

So when user1 reads errors, he's actually reading the errors by user2, who just completed logging the error. Your UploadServlet may have a member ErrorStatus object: that would create the problem.

Good luck!

here's a good tutorial: http://www.informit.com/articles/article.aspx?p=23947

Upvotes: 0

mindas
mindas

Reputation: 26713

I think your problem lies in this line:

messages = other.getMessages();

Basically this means that two different ErrorStatus objects are sharing the same List and modifications to one list would affect the other and vice versa. This line should look like

messages = new ArrayList<ErrorMessage>(other.getMessages());

This way, the list will (initially) contain the same elements but will be immune to modification side effects.

Upvotes: 1

Related Questions