Is there any flaw in this code?

I wrote a servlet, but the servlet is not yet in production stage.

I've added a counter in the Filter of the servlet, so that when number of concurrent request reach a limit, no more people can be accepted. I worry some marginal case, for example: Say the system has already reached 49 concurrent request(50 is max), and in the synchronized block it makes the boolean variable "ok" to True, then at the next instance, multiple thread see that the servlet is available and rush into that and break the limit.

Please help to check this code if there is any flaw:

public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException {
        // TODO Auto-generated method stub
        // place your code here

        // pass the request along the filter chain
        conditionalInfoLog(logEnabled, "Incoming request...");
        conditionalInfoLog(logEnabled, "Number of concurrent request(s): " + count);
        boolean ok;
        synchronized (lock) {
            ok = count < limit;
            if(ok){
                count++;
            }
        }
        if (ok) {
            try{
                // let the request through and process as usual
                conditionalInfoLog(logEnabled, "Request accepted and processing, number of concurrent request(s): " + count);
                chain.doFilter(request, response);

            }catch(Exception ex){
                conditionalErrorLog(logEnabled, ex.getMessage());
                String xmlStr = genXmlErrMsg(ex.getMessage());
                response.setContentType("text/xml");
                response.getWriter().print(xmlStr);
            }finally{
                synchronized (lock) {开发者_开发百科
                    count--;
                }
                conditionalInfoLog(logEnabled, "End of request. Number of concurrent request(s): " + count);
            }
        } else {
            // handle limit case, e.g. return status code 503
            conditionalInfoLog(logEnabled, busyMsg);
            String xmlStr = genXmlErrMsg(busyMsg);
            response.setContentType("text/xml");
            response.getWriter().print(xmlStr);
        }

    }

I'd rather configure it at servletcontainer level than trying to fiddle something together. A bit decent servletcontainer is thoroughly tested and surely production ready.

Tomcat, for example, has a maxThreads attribute exactly for this purpose. You can set it in the <Connector> element in server.xml.

<Connector maxThreads="50" ...>

This puts a limit on amount of simultaneous requests (which defaults to 200 by the way). So when there's an 51st request, it's just put in a queue (whose length is configureable by acceptCounts attribute by the way) until the first is ready. This is also a bit more user friendly than a 503.

See also:

• Tomcat 7.0 documentation - The HTTP Connector

It is correct, but you are basically reinventing a java.util.concurrent.Semaphore:

class MyFilter {
  final Semaphore permits = new Semaphore(50);

  public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException {
    if (permits.tryAcquire()) {
        try {
          … // execute
        } finally {
          permits.release();
        }
    } else {
      … // handle limit case, e.g. return status code 503
    }
  }
}

This class is much more efficient than your hand-rolled solution and better expresses your intent anyway.

At most one thread at a time is allowed synchronize on an object. If several threads try to "rush into" a synchronized block, only one will make it. Also, each thread will have its local copy of ok, and each thread will have to make it through this block:

synchronized (lock) {
    ok = count < limit;
    if (ok) {
        count++;
    }
}

before it can test the value of it's ok variable. So I think this is pretty safe.

It's good that you also synchronize the decrement of count later on; this will ensure that the value of count will be flushed to memory before any other thread can enter a block that synchronizes on lock.