Details

    • Type: Bug
    • Status: Resolved (View Workflow)
    • Priority: Blocker
    • Resolution: Fixed
    • Component/s: remoting
    • Labels:
      None
    • Similar Issues:
    • Epic Link:

      Description

      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

        Attachments

          Issue Links

            Activity

            Hide
            scm_issue_link 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.

            Show
            scm_issue_link 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.
            Hide
            scm_issue_link 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

            Show
            scm_issue_link 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
            Hide
            scm_issue_link 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

            Show
            scm_issue_link 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
            Hide
            scm_issue_link 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

            Show
            scm_issue_link 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
            Hide
            teilo James Nord added a comment -

            will be released when Oleg Nenashev releases remoting 3

            Show
            teilo James Nord added a comment - will be released when Oleg Nenashev releases remoting 3
            Hide
            oleg_nenashev Oleg Nenashev added a comment -

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

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

              People

              • Assignee:
                teilo James Nord
                Reporter:
                teilo James Nord
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: