Filter request code handler

Question

I use the following code which works OK, the code is filtering requests and in case there are too many requests it sends some logs/errors since I'm very new to Java my question if there is a better way to write it especially the while statement ...

 public final class filterBu implements Filter {

        private static AtomicInteger reqcnt;
        private static final String MAX_CUNCCURENT = "maxConcurrent";
        private static final Logger logger = LoggerFactory.getLogger(ContentValidationFilter.class);
        private static final String EXCEED_MAX_REQUESTS_MSG = "maximum reaced";
        private static int maxCunnccurent;
        private static final int TOO_MANY_REQUESTS = 429;


        @Override
        public void doFilter(ServletRequest req, ServletResponse resp, FilterChain chain) throws IOException, ServletException {
            while (true) {
                int value = reqcnt.get();
                if (value >= maxCunnccurent) {
                    ((HttpServletResponse) resp).sendError(TOO_MANY_REQUESTS, EXCEED_MAX_REQUESTS_MSG);
                    return;
                }
                if (reqcnt.compareAndSet(value, value + 1)) {
                    break;
                }
            }
            try {
                chain.doFilter(req, resp);
            } finally {
                reqcnt.decrementAndGet();
            }

        }

        @Override
        public void init(FilterConfig config) throws ServletException, NumberFormatException {
            maxCunnccurent = Integer.parseInt(config.getInitParameter(MAX_CUNCCURENT));
            if (maxCunnccurent < 1) {
                logger.error("excced limit"); 
            }
            reqcnt = new AtomicInteger(0);
        }

    }

Answer 1

Replace the AtomicInteger with a Semaphore holding maxCunnccurent (maxConcurrent, right?) permits. So :

reqcnt = new Semaphore(maxCunnccurent);

Now, each time you get a filter request, you try to get a permit. If you do get a permit, you go ahead and filter, making sure that afterwards you release the permit again. If you don't get a permit, you send your error response :

boolean acquired = reqcnt.tryAcquire();
if (!acquired) {
    ((HttpServletResponse) resp).sendError(TOO_MANY_REQUESTS, EXCEED_MAX_REQUESTS_MSG);
    return;
}
try {
    chain.doFilter(req, resp);
} finally {
    reqcnt.release();
}

Answer 2

public final class filterBu implements Filter {

In Java, class names start with a capital letter. It should be FilterBu instead of filterBu.

What does Bu mean? Choose a proper name for your classes. Don't be disappointed if your first name choice is not perfect yet, since choosing good names is difficult. But you can surely find a better name than Bu.

    private static AtomicInteger reqcnt;

This variable must not be static. Imagine you had 17 instances of this filter class. How many counters should you have, 1 or 17? If your answer is 17, remove the static.

    private static final String MAX_CUNCCURENT = "maxConcurrent";

You should run your code through a spell checker, since you make many typos. This one should be concurrent. And even if you make spelling mistakes, you should at least be consistent. The name of this constant and its value must match.

    private static final Logger logger = LoggerFactory.getLogger(ContentValidationFilter.class);

A static logger should always be defined with the enclosing class as argument. Imagine you get a log message saying it's from ContentValidationFilter. Where would you suspect it -- in the ContentValidationFilter class, or in the completely unrelated filterBu class?

    private static final String EXCEED_MAX_REQUESTS_MSG = "maximum reaced";

It's reached.

    private static int maxCunnccurent;

Another variation on concurrent.

    private static final int TOO_MANY_REQUESTS = 429;

    @Override
    public void init(FilterConfig config) throws ServletException, NumberFormatException {

You don't need to declare these exceptions to be thrown, for two different reasons. The ServletException is never thrown by your code, while the NumberFormatException is a RuntimeException, which doesn't have to be declared.

        maxCunnccurent = Integer.parseInt(config.getInitParameter(MAX_CUNCCURENT));
        if (maxCunnccurent < 1) {
            logger.error("excced limit"); 

Another variation on limit exceeded. Furthermore, the error message is misleading, since there is no limit that could be exceeded. This one should rather say logger.error("Invalid value for maxConcurrent: {}", maxConcurrent).

But you should not log anything here at all. Just throw an exception. Because when you don't, maxConcurrent will be initialized to 0, allowing 0 concurrent connections. This in turn means that every connection attempt will get a 429 error. In such a case it is better to have an exception stack trace in the log file instead of nothing.

        }
        reqcnt = new AtomicInteger(0);
    }

}