Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-45023

Channel#call() should reject requests if the channel is being closed

    XMLWordPrintable

Details

    Description

      It is a follow-up to https://github.com/jenkinsci/jenkins/pull/2923#discussion_r123202990 . The most of Remoting calls in Jenkins should not be really executed when the channel is shutting down. In our case it was a Launcher for Node (JENKINS-38527) 

       

      I expected channel.call() to handle it, but it does not... The only handler is https://github.com/jenkinsci/remoting/blob/a762e0703a04ae41fbd413b2379d57f80fc282b2/src/main/java/hudson/remoting/Channel.java#L604-L605 , which effectively checks for the closed channel after a bunch of operations. 

IMHO Channel should provide a new API for user-space calls which should not be performed in parallel with close. Making it a default behavior seems to be a breaking change though, needs much testing

       

      Attachments

        Issue Links

          depends on

          Improvement - An improvement or enhancement to an existing feature or task. JENKINS-43985 Update Remoting Java requirement to Java 8

          • Minor - Minor loss of function, or other problem where easy workaround is present.
          • Resolved
          is related to

          Bug - A problem which impairs or prevents the functions of the product. JENKINS-46067 Pipeline task scheduled on uninitialized node

          • Major - Major loss of function.
          • Open

          Bug - A problem which impairs or prevents the functions of the product. JENKINS-45294 Remoting should reject RPCRequests over the closing channel

          • Minor - Minor loss of function, or other problem where easy workaround is present.
          • Resolved

          Bug - A problem which impairs or prevents the functions of the product. JENKINS-43038 Intermittent error "Cannot contact node123: java.lang.InterruptedException " in jenkins

          • Major - Major loss of function.
          • Closed
          links to

          Web Link CloudBees Internal CJP-6857

          Web Link CloudBees Internal OSS-2368

          Web Link https://github.com/jenkinsci/remoting/pull/175

          Activity

            Ascending order - Click to sort in descending order
            oleg_nenashev Oleg Nenashev added a comment -

            In order to do it properly, we likely need a new default implementation in the VirtualChannel interface. Hence we need to migrate Remoting to Java 8 (JENKINS-43985).

            oleg_nenashev Oleg Nenashev added a comment - In order to do it properly, we likely need a new default implementation in the VirtualChannel interface. Hence we need to migrate Remoting to Java 8 ( JENKINS-43985 ).
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Oleg Nenashev
            Path:
            src/main/java/hudson/remoting/Channel.java
            src/main/java/hudson/remoting/ChannelClosedException.java
            src/main/java/hudson/remoting/Request.java
            src/main/java/hudson/remoting/UserRequest.java
            src/main/java/hudson/remoting/VirtualChannel.java
            src/test/java/hudson/remoting/ChannelTest.java
            http://jenkins-ci.org/commit/remoting/67edc4bc4896ce99f3246b6b7cac25fbb94f71b3
            Log:
            JENKINS-45023 - Prevent execution of commands on closed or beingClosed channels (#175)

            • JENKINS-45023 - Prevent execution of commands on closed or beingClosed channels

            This is a major update of Request execution logic in remoting Channels, which should improve stability of the channel and prevent hanging of commands if the channel gets closed.

            • [x] - `Channel#close()` does not always wait of synchronization to happen. There is a sender status check before the lock gets acquired. TODO: find an issue for that
            • [x] - `Channel#isClosingOrClosed()` now returns `null` once the first `Channel#close()` command arrives, we do not even wait till it acquires the instance lock. The API is [used outside Remoting](https://github.com/search?q=org%3Ajenkinsci+isClosingOrClosed&type=Code), but it seems that the change is correct in that cases
            • [x] - `Channel#call()` and `Channel#callAsync()` now fail if the channel `isClosingOrClosed()`. These calls implement `UserRequest`, and I do not think there is a valid case for even trying any user-space request
            • [x] - Offer new API in `hudson.remoting.Request`, which allows checking the channel state before invoking a call. By default it just checks if the channel is closed (just “fail fast” without command initialization)
            • [x] - Implement the new API in `UserRequest`, to prevent low-level API calls on a channel, which `isClosingOrClosed()`
            • JENKINS-45023 - Chanel#terminate() should also immediately set the closeRequested flag
            • JENKINS-45023 - Use ChannelClosedException when channel is being closed and cannot accept commands
            • JENKINS-45023 - Add functional tests for the deadlocked channel
            • JENKINS-45023 - UserRequest constructor should not hang when the channel shutdown is pendind && the lock cannot be acquired
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Oleg Nenashev Path: src/main/java/hudson/remoting/Channel.java src/main/java/hudson/remoting/ChannelClosedException.java src/main/java/hudson/remoting/Request.java src/main/java/hudson/remoting/UserRequest.java src/main/java/hudson/remoting/VirtualChannel.java src/test/java/hudson/remoting/ChannelTest.java http://jenkins-ci.org/commit/remoting/67edc4bc4896ce99f3246b6b7cac25fbb94f71b3 Log: JENKINS-45023 - Prevent execution of commands on closed or beingClosed channels (#175) JENKINS-45023 - Prevent execution of commands on closed or beingClosed channels This is a major update of Request execution logic in remoting Channels, which should improve stability of the channel and prevent hanging of commands if the channel gets closed. [x] - `Channel#close()` does not always wait of synchronization to happen. There is a sender status check before the lock gets acquired. TODO: find an issue for that [x] - `Channel#isClosingOrClosed()` now returns `null` once the first `Channel#close()` command arrives, we do not even wait till it acquires the instance lock. The API is [used outside Remoting] ( https://github.com/search?q=org%3Ajenkinsci+isClosingOrClosed&type=Code ), but it seems that the change is correct in that cases [x] - `Channel#call()` and `Channel#callAsync()` now fail if the channel `isClosingOrClosed()`. These calls implement `UserRequest`, and I do not think there is a valid case for even trying any user-space request [x] - Offer new API in `hudson.remoting.Request`, which allows checking the channel state before invoking a call. By default it just checks if the channel is closed (just “fail fast” without command initialization) [x] - Implement the new API in `UserRequest`, to prevent low-level API calls on a channel, which `isClosingOrClosed()` JENKINS-45023 - Address Javadoc comments from @jglick JENKINS-45023 - Chanel#terminate() should also immediately set the closeRequested flag JENKINS-45023 - Use ChannelClosedException when channel is being closed and cannot accept commands JENKINS-45023 - Add functional tests for the deadlocked channel JENKINS-45023 - UserRequest constructor should not hang when the channel shutdown is pendind && the lock cannot be acquired
            oleg_nenashev Oleg Nenashev added a comment -

            The fix has been integrated towards Remoting 3.11 and Jenkins 2.76

            oleg_nenashev Oleg Nenashev added a comment - The fix has been integrated towards Remoting 3.11 and Jenkins 2.76
            olivergondza Oliver Gondža added a comment -

            oleg_nenashev, 3.11 is too new to be backported IMO and I have not found a stable remoting branch on GH. I am wondering if remoting-lts-candidate has any significance for core backporting process.

            olivergondza Oliver Gondža added a comment - oleg_nenashev , 3.11 is too new to be backported IMO and I have not found a stable remoting branch on GH. I am wondering if remoting-lts-candidate has any significance for core backporting process.
            oleg_nenashev Oleg Nenashev added a comment -

            3.11 is not a subject for LTS for sure due to Java requirements. These things are for .2, maybe . remoting-lts-candidate is a marker for.me so that I can spin 3.10.1

            oleg_nenashev Oleg Nenashev added a comment - 3.11 is not a subject for LTS for sure due to Java requirements. These things are for .2, maybe . remoting-lts-candidate is a marker for.me so that I can spin 3.10.1
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Oleg Nenashev
            Path:
            src/main/java/hudson/remoting/Channel.java
            src/main/java/hudson/remoting/ChannelClosedException.java
            src/main/java/hudson/remoting/Request.java
            src/main/java/hudson/remoting/UserRequest.java
            src/main/java/hudson/remoting/VirtualChannel.java
            src/test/java/hudson/remoting/ChannelTest.java
            http://jenkins-ci.org/commit/remoting/a1294d6fd5e0053098a532488aca02586c02a887
            Log:
            JENKINS-45023 - Prevent execution of commands on closed or beingClosed channels (#175)

            • JENKINS-45023 - Prevent execution of commands on closed or beingClosed channels

            This is a major update of Request execution logic in remoting Channels, which should improve stability of the channel and prevent hanging of commands if the channel gets closed.

            • [x] - `Channel#close()` does not always wait of synchronization to happen. There is a sender status check before the lock gets acquired. TODO: find an issue for that
            • [x] - `Channel#isClosingOrClosed()` now returns `null` once the first `Channel#close()` command arrives, we do not even wait till it acquires the instance lock. The API is [used outside Remoting](https://github.com/search?q=org%3Ajenkinsci+isClosingOrClosed&type=Code), but it seems that the change is correct in that cases
            • [x] - `Channel#call()` and `Channel#callAsync()` now fail if the channel `isClosingOrClosed()`. These calls implement `UserRequest`, and I do not think there is a valid case for even trying any user-space request
            • [x] - Offer new API in `hudson.remoting.Request`, which allows checking the channel state before invoking a call. By default it just checks if the channel is closed (just “fail fast” without command initialization)
            • [x] - Implement the new API in `UserRequest`, to prevent low-level API calls on a channel, which `isClosingOrClosed()`
            • JENKINS-45023 - Chanel#terminate() should also immediately set the closeRequested flag
            • JENKINS-45023 - Use ChannelClosedException when channel is being closed and cannot accept commands
            • JENKINS-45023 - Add functional tests for the deadlocked channel
            • JENKINS-45023 - UserRequest constructor should not hang when the channel shutdown is pendind && the lock cannot be acquired
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Oleg Nenashev Path: src/main/java/hudson/remoting/Channel.java src/main/java/hudson/remoting/ChannelClosedException.java src/main/java/hudson/remoting/Request.java src/main/java/hudson/remoting/UserRequest.java src/main/java/hudson/remoting/VirtualChannel.java src/test/java/hudson/remoting/ChannelTest.java http://jenkins-ci.org/commit/remoting/a1294d6fd5e0053098a532488aca02586c02a887 Log: JENKINS-45023 - Prevent execution of commands on closed or beingClosed channels (#175) JENKINS-45023 - Prevent execution of commands on closed or beingClosed channels This is a major update of Request execution logic in remoting Channels, which should improve stability of the channel and prevent hanging of commands if the channel gets closed. [x] - `Channel#close()` does not always wait of synchronization to happen. There is a sender status check before the lock gets acquired. TODO: find an issue for that [x] - `Channel#isClosingOrClosed()` now returns `null` once the first `Channel#close()` command arrives, we do not even wait till it acquires the instance lock. The API is [used outside Remoting] ( https://github.com/search?q=org%3Ajenkinsci+isClosingOrClosed&type=Code ), but it seems that the change is correct in that cases [x] - `Channel#call()` and `Channel#callAsync()` now fail if the channel `isClosingOrClosed()`. These calls implement `UserRequest`, and I do not think there is a valid case for even trying any user-space request [x] - Offer new API in `hudson.remoting.Request`, which allows checking the channel state before invoking a call. By default it just checks if the channel is closed (just “fail fast” without command initialization) [x] - Implement the new API in `UserRequest`, to prevent low-level API calls on a channel, which `isClosingOrClosed()` JENKINS-45023 - Address Javadoc comments from @jglick JENKINS-45023 - Chanel#terminate() should also immediately set the closeRequested flag JENKINS-45023 - Use ChannelClosedException when channel is being closed and cannot accept commands JENKINS-45023 - Add functional tests for the deadlocked channel JENKINS-45023 - UserRequest constructor should not hang when the channel shutdown is pendind && the lock cannot be acquired

            People

              oleg_nenashev Oleg Nenashev
              oleg_nenashev Oleg Nenashev
              Votes:
              2 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: