• Icon: Bug Bug
    • Resolution: Fixed
    • Icon: Blocker Blocker
    • remoting
    • None

      When JNLP v4 protocol is in use with non blocking IO when there is more than one buffers worth of data to read we re-add the OP_READ to the selector for each read buffer even though we drain until we have read all the data available.
      This causes a new Thread to be created to handles the callback (when the selector wakes up) - but each thread will be blocked as the current thread is still reading and holding the read lock.

      see https://github.com/jenkinsci/remoting/blob/c3e675c9f1dc29a8fd99eca191c1ce1e5ebb2a7e/src/main/java/org/jenkinsci/remoting/protocol/impl/NIONetworkLayer.java#L150-L155

          [JENKINS-38690] JNLPv4 Thread storm

          Code changed in jenkins
          User: James Nord
          Path:
          src/main/java/org/jenkinsci/remoting/protocol/impl/NIONetworkLayer.java
          http://jenkins-ci.org/commit/remoting/751cb2e84945cdaf9d00cafd24d983dd83f1196a
          Log:
          JENKINS-38690 do not cause a IOHub Thread Storm

          We should only add the READ interested OPS when we have fully drained the
          read queue.

          In the case we had a lot of data to read (where lots >> 8192 bytes) we
          would read a buffers worth of data add READ to the interested opps then
          read another buffer in the same thread. If we where reading 81920 bytes
          for example this would be 9 calls to add READ OPs - which would run on the
          selctor thread and cause an immediate wakeup to be called followed by a
          new onReady().
          However the onReady will be added as a defered callback and the ops
          removed, but the read thread is still going and as it takes time will call
          back again to add interested ops... which will wake the selector see that
          READ ops are valid and cause a new deffered callback...

          So do not add read interested ops inside the loop, add it only when we
          have drained the buffer.

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: James Nord Path: src/main/java/org/jenkinsci/remoting/protocol/impl/NIONetworkLayer.java http://jenkins-ci.org/commit/remoting/751cb2e84945cdaf9d00cafd24d983dd83f1196a Log: JENKINS-38690 do not cause a IOHub Thread Storm We should only add the READ interested OPS when we have fully drained the read queue. In the case we had a lot of data to read (where lots >> 8192 bytes) we would read a buffers worth of data add READ to the interested opps then read another buffer in the same thread. If we where reading 81920 bytes for example this would be 9 calls to add READ OPs - which would run on the selctor thread and cause an immediate wakeup to be called followed by a new onReady(). However the onReady will be added as a defered callback and the ops removed, but the read thread is still going and as it takes time will call back again to add interested ops... which will wake the selector see that READ ops are valid and cause a new deffered callback... So do not add read interested ops inside the loop, add it only when we have drained the buffer.

          Code changed in jenkins
          User: James Nord
          Path:
          Jenkinsfile
          http://jenkins-ci.org/commit/remoting/48104c792cb50f02ffd05f968760a38102bf47d2
          Log:
          Merge remote-tracking branch 'origin/master' into JENKINS-38690

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: James Nord Path: Jenkinsfile http://jenkins-ci.org/commit/remoting/48104c792cb50f02ffd05f968760a38102bf47d2 Log: Merge remote-tracking branch 'origin/master' into JENKINS-38690

          Code changed in jenkins
          User: Stephen Connolly
          Path:
          src/main/java/org/jenkinsci/remoting/protocol/impl/NIONetworkLayer.java
          http://jenkins-ci.org/commit/remoting/618a16a823abb11bbb256733218421ea526f5106
          Log:
          JENKINS-38690 Rework to use a more performant loop

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Stephen Connolly Path: src/main/java/org/jenkinsci/remoting/protocol/impl/NIONetworkLayer.java http://jenkins-ci.org/commit/remoting/618a16a823abb11bbb256733218421ea526f5106 Log: JENKINS-38690 Rework to use a more performant loop

          Code changed in jenkins
          User: Stephen Connolly
          Path:
          src/main/java/org/jenkinsci/remoting/protocol/impl/NIONetworkLayer.java
          http://jenkins-ci.org/commit/remoting/9c40e36c1cd708a4a92765a4e42a28f5ad97382e
          Log:
          Merge pull request #117 from stephenc/jenkins-38690

          JENKINS-38690 Do not cause a Thread storm in IOHub

          Compare: https://github.com/jenkinsci/remoting/compare/b95b144ca1e3...9c40e36c1cd7

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Stephen Connolly Path: src/main/java/org/jenkinsci/remoting/protocol/impl/NIONetworkLayer.java http://jenkins-ci.org/commit/remoting/9c40e36c1cd708a4a92765a4e42a28f5ad97382e Log: Merge pull request #117 from stephenc/jenkins-38690 JENKINS-38690 Do not cause a Thread storm in IOHub Compare: https://github.com/jenkinsci/remoting/compare/b95b144ca1e3...9c40e36c1cd7

          James Nord added a comment -

          will be released when oleg_nenashev releases remoting 3

          James Nord added a comment - will be released when oleg_nenashev releases remoting 3

          Oleg Nenashev added a comment -

          Fixed within 3.0 beta testing. No need to release note it

          Oleg Nenashev added a comment - Fixed within 3.0 beta testing. No need to release note it

            Unassigned Unassigned
            teilo James Nord
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Created:
              Updated:
              Resolved: