[BUG] JSQLParser Version : 5.1: CCJSqlParserUtil.parse timeout Not thread-safe

[relufi]

The CCJSqlParser.interrupted field should use AtomicBoolean

[relufi]

It is cross-thread

parser.interrupted = true;

It is possible for ! interrupted to be inlined into a register, resulting in interrupted always being false

[relufi]

It is also unwise to use an ExecutorService to implement timeouts, forcing the parsing work to be limited by the number of threads in the thread pool

This is an ideal way of implementation, only need to use a global thread ScheduledThreadPoolExecutor whether to mark a timeout, which will help the parse SQL is limited by the ExecutorService number of threads

public static ScheduledThreadPoolExecutor scheduledThreadPoolExecutor = new ScheduledThreadPoolExecutor(1);
    private static final VarHandle IVH;
    static {
        try {
            IVH = MethodHandles.lookup().findVarHandle(CCJSqlParser.class, "interrupted", boolean.class);
        } catch (NoSuchFieldException | IllegalAccessException e) {
            throw new RuntimeException(e);
        }
    }
    public static Statement parseWithTimeOut(CCJSqlParser parser) throws ParseException {
        ScheduledFuture<?> schedule = scheduledThreadPoolExecutor.schedule(
                () ->{
                    //todo or use AtomicBoolean.setOpaque  or use volatile  But there is an additional overhead
                    //todo It's not actually valid  Because CCJSqlParser gets interrupted as a normal variable
                    IVH.setOpaque(parser, true)
                },
                4000, TimeUnit.MILLISECONDS
        );
        try {
            return parser.Statement();
        } finally {
            schedule.cancel(false);
        }
    }

[relufi]

Or just add CCJSqlParser! interrupted is replaced with a check to see if the current time is greater than the timeout, which allows us to disconnect from the thread altogether

[manticore-projects]

Greetings! Thank you very much for you input which I appreciate a lot. Although your comments are a bit scarce to help my understanding.

1. the parser is not threadsafe by design (although I agree that an AtomicBoolean won't hurt)
2. you methods look a lot like JDK17+ (while we are yet on JDK11)
3. PRs are welcome of course!

[relufi]

Thank you for your reply. I'm sorry that I don't know English and thus I'm using translation software. As a result, what you see might not be very understandable.

VarHandle and AtomicBoolean setOpaque/getOpaque were introduced in JDK 9. For versions prior to JDK 9, using volatile is also acceptable, but it may incur unnecessary runtime overhead. Here, only the compile-time barrier with Opaque semantics is needed to ensure that the variable is not inlined and optimized by the compiler to be stored in a register.

It is unreasonable to use ExecutorService to implement timeout at present. For example, in the case of web application, the caller is Tomcat with over 100 threads. When handling requests, if a thread pool with less than 100 threads is used to implement timeout processing for jsqlparser, then jsqlparser will become a bottleneck of the system. The concurrency quantity is limited by jsqlparse

The issue can be resolved by using a ScheduledThreadPoolExecutor with only one thread. By parsing the SQL statements directly within the user's calling thread and determining whether there is a timeout in the schedule thread, this approach will not be constrained by the number of threads in the passed-in thread pool.