views:

76

answers:

3

Hello, looking at http://download.eclipse.org/jetty/stable-7/xref/com/acme/ChatServlet.html, I don't seem to understand why there needs to be a synchronization block in a synchronized method, like so:

private synchronized void chat(HttpServletRequest request,HttpServletResponse response,String username,String message)
throws IOException
{
    Map<String,Member> room=_rooms.get(request.getPathInfo());
    if (room!=null)
    {
        // Post chat to all members
        for (Member m:room.values())
        {
            synchronized (m)
            {
                m._queue.add(username); // from
                m._queue.add(message);  // chat

                // wakeup member if polling
                if (m._continuation!=null)
                {
                    m._continuation.resume();
                    m._continuation=null;
                }
            }
        }
    }

Why does m need to be synchronized (again?) if the whole method is already thread-safe?

Thank you for any insight.

+4  A: 

The synchronized method "chat(...)" synchronizes on it's instance object whereas the synchronized(m) synchronizes on the "m" object - so they are synchronizing on two different objects. Basically it's making sure that some other servlet object isn't messing with the same Member instance at the same time.

Marc Novakowski
Thanks, I understand, but just out of curiosity.. why isn't the _rooms object also synchronized?
David Titarenco
Probably because _rooms is only modified by other synchronized methods in the same class, so as long as you're accessing it in a synchronized method you can be sure that nobody else is adding to or removing items from it. Ideally it would be defined as "private" because right now it is package-private meaning any other classes in the package (or subclasses) could access it.
Marc Novakowski
Thanks for your comment, but then I need to go back to my original question. Why is synchronizing 'm' necessary? It's only used in already-synchronized methods (much like _rooms). Or is synchronizing 'm' just a precaution?
David Titarenco
I'm not too familiar with this class or even Jetty Continuations which is what it is demoing, but in my opinion I don't think it's necessary to synchronize on both the servlet instance (at the method level) and on the Member object. Synchronizing all the methods may be "safer" but it also runs the risk of unnecessary blocking when multiple clients are hitting the servlet at the same time. Synchronizing on the "room" and "member" objects only (and making them private) I think would be the better approach.
Marc Novakowski
Excellent response, and exactly what I was thinking. Thank you so much for clarifying!
David Titarenco
+1  A: 

When whole method is synchronized the lock is obtained on the this object. But the synchronized block obtains lock only on the member currently being used in iteration.

Chandru
A: 

The synchronization is on different locks.

The synchronized keyword at the method definition means that other code that synchronizes on this cannot run run in parallel to the method.

The synchronized(m) scope means that other code that synchronizes on m cannot run in parallel to the loop.

Itay