Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework AFSelector to normalize and simplify its locking #146

Commits on Oct 28, 2023

  1. Fix file descriptor leak under high concurrency

    With our PoC at
    https://github.com/kevink-sq/jetty-concurrency-issue-poc/tree/575469b3453e7b09d9d60f6460cdab8117b8e773,
    we noticed that there is unbounded growth of file descriptors when
    running our simple load test:
    
    ```bash
    seq 1 15000 | xargs -P1500 -I{} curl --unix-socket ./junix-ingress.sock http://localhost/process
    ```
    
    Leading to timeouts from curl and timeout exceptions in jetty:
    
    ```
    java.util.concurrent.TimeoutException: Idle timeout expired: 32422/30000 ms
            at org.eclipse.jetty.io.IdleTimeout.checkIdleTimeout(IdleTimeout.java:170)
            at org.eclipse.jetty.io.IdleTimeout.idleCheck(IdleTimeout.java:112)
            at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539)
            at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
            at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304)
            at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
            at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
            at java.base/java.lang.Thread.run(Thread.java:833)
    ```
    
    This patch fixes the issue by ensuring that keys with the `OP_INVALID`
    bit are still tracked in the selected key set and cancelled accordingly
    
    Changes:
    * Add invalid keys to `selectedKeysSet` in `setOpsReady` and mark as
      cancelled, ensuring that the fd is cleaned up timely
    * Switch from `cancelNoRemove` to `cancel` now that the registered keys
      are modified separately in the select implementation
    * Prefer `!key.isValid()` over `key.hasOpInvalid()` since the former is
      a superset of the latter
    * In `implCloseSelector`, cancel the keys before clearing
      `keysRegistered` since `keys()` is backed by `keysRegistered`
    * Store cancelled keys in a deque instead of a hash set, which should
      be a little more efficient
    * Remove dead code
    ThePumpingLemma committed Oct 28, 2023
    Configuration menu
    Copy the full SHA
    6818639 View commit details
    Browse the repository at this point in the history

Commits on Nov 7, 2023

  1. Rename prepareRemove and performRemoveto cancel and

    `processDeregisterQueue`, respectively
    
    This naming convention more closely aligns with the selector
    implementations in Java's standard library
    ThePumpingLemma committed Nov 7, 2023
    Configuration menu
    Copy the full SHA
    a2866c6 View commit details
    Browse the repository at this point in the history
  2. Configuration menu
    Copy the full SHA
    b9695d6 View commit details
    Browse the repository at this point in the history
  3. Rework AFSelector to normalize and simplify its locking

    * Place bulk of `select0` operations under `this` and
      `publicSelectedKeys` locks to reduce locking/unlocking thrash given
      frequency of locking operations
    * Lock `publicSelectedKeys` instead of `selectedKeys` since that is what
      clients of AFSelector are locking on
    * Eschew uses of synchronized methods in favor of locking `this` via
      synchronized blocks
    * Use assertions to check lock invariants
    ThePumpingLemma committed Nov 7, 2023
    Configuration menu
    Copy the full SHA
    1346d2c View commit details
    Browse the repository at this point in the history