Uploaded image for project: 'Flink'
  1. Flink
  2. FLINK-6143

Unprotected access to this.flink in LocalExecutor#endSession()

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.3.0
    • Fix Version/s: 1.3.0
    • Component/s: Client
    • Labels:
      None

      Description

        public void endSession(JobID jobID) throws Exception {
          LocalFlinkMiniCluster flink = this.flink;
          if (flink != null) {
      

      The flink field is not declared volatile and access to this.flink doesn't hold the LocalExecutor.lock

        Issue Links

          Activity

          Hide
          mingleizhang mingleizhang added a comment - - edited

          Ted Yu Hi, Could you please assign this jira to me ? I will work on this soon. Thanks.

          Show
          mingleizhang mingleizhang added a comment - - edited Ted Yu Hi, Could you please assign this jira to me ? I will work on this soon. Thanks.
          Hide
          Zentol Chesnay Schepler added a comment -

          mingleizhang I've given you contributor permissions; you can now assign issues to yourself.

          Show
          Zentol Chesnay Schepler added a comment - mingleizhang I've given you contributor permissions; you can now assign issues to yourself.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user zhangminglei opened a pull request:

          https://github.com/apache/flink/pull/3710

          FLINK-6143 [clients] Fix unprotected access to this.flink in LocalE…

          …xecutor#endSession.

          Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
          If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the [How To Contribute guide](http://flink.apache.org/how-to-contribute.html).
          In addition to going through the list, please provide a meaningful description of your changes.

          • [ ] General
          • The pull request references the related JIRA issue ("[FLINK-XXX] Jira title text")
          • The pull request addresses only one issue
          • Each commit in the PR has a meaningful commit message (including the JIRA id)
          • [ ] Documentation
          • Documentation has been added for new functionality
          • Old documentation affected by the pull request has been updated
          • JavaDoc for public methods has been added
          • [ ] Tests & Build
          • Functionality added by the pull request is covered by tests
          • `mvn clean verify` has been executed successfully locally or a Travis build has passed

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/zhangminglei/flink flink_6143

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/flink/pull/3710.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #3710


          commit 83904c4e736d17cb32ffcbf1d2f90b5521837284
          Author: zhangminglei <zml13856086071@163.com>
          Date: 2017-04-11T15:29:50Z

          FLINK-6143 [clients] Fix unprotected access to this.flink in LocalExecutor#endSession.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user zhangminglei opened a pull request: https://github.com/apache/flink/pull/3710 FLINK-6143 [clients] Fix unprotected access to this.flink in LocalE… …xecutor#endSession. Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration. If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the [How To Contribute guide] ( http://flink.apache.org/how-to-contribute.html ). In addition to going through the list, please provide a meaningful description of your changes. [ ] General The pull request references the related JIRA issue (" [FLINK-XXX] Jira title text") The pull request addresses only one issue Each commit in the PR has a meaningful commit message (including the JIRA id) [ ] Documentation Documentation has been added for new functionality Old documentation affected by the pull request has been updated JavaDoc for public methods has been added [ ] Tests & Build Functionality added by the pull request is covered by tests `mvn clean verify` has been executed successfully locally or a Travis build has passed You can merge this pull request into a Git repository by running: $ git pull https://github.com/zhangminglei/flink flink_6143 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3710.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #3710 commit 83904c4e736d17cb32ffcbf1d2f90b5521837284 Author: zhangminglei <zml13856086071@163.com> Date: 2017-04-11T15:29:50Z FLINK-6143 [clients] Fix unprotected access to this.flink in LocalExecutor#endSession.
          Hide
          mingleizhang mingleizhang added a comment -

          Chesnay Schepler Thanks for your help and the pr is ready now. Could you please help me review ? Many thanks go out to you.

          Show
          mingleizhang mingleizhang added a comment - Chesnay Schepler Thanks for your help and the pr is ready now. Could you please help me review ? Many thanks go out to you.
          Hide
          mingleizhang mingleizhang added a comment -

          Stephan Ewen , Hi, Could you please take some time review this patch ? Many thanks go out to you.

          Show
          mingleizhang mingleizhang added a comment - Stephan Ewen , Hi, Could you please take some time review this patch ? Many thanks go out to you.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zentol commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3710#discussion_r111663699

          — Diff: flink-clients/src/main/java/org/apache/flink/client/LocalExecutor.java —
          @@ -59,7 +59,7 @@
          private final Object lock = new Object();

          /** The mini cluster on which to execute the local programs */

          • private LocalFlinkMiniCluster flink;
            + private volatile LocalFlinkMiniCluster flink;
              • End diff –

          If we guard all accesses with locks this doesn't have to be volatile.

          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on a diff in the pull request: https://github.com/apache/flink/pull/3710#discussion_r111663699 — Diff: flink-clients/src/main/java/org/apache/flink/client/LocalExecutor.java — @@ -59,7 +59,7 @@ private final Object lock = new Object(); /** The mini cluster on which to execute the local programs */ private LocalFlinkMiniCluster flink; + private volatile LocalFlinkMiniCluster flink; End diff – If we guard all accesses with locks this doesn't have to be volatile.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zhangminglei commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3710#discussion_r111666373

          — Diff: flink-clients/src/main/java/org/apache/flink/client/LocalExecutor.java —
          @@ -59,7 +59,7 @@
          private final Object lock = new Object();

          /** The mini cluster on which to execute the local programs */

          • private LocalFlinkMiniCluster flink;
            + private volatile LocalFlinkMiniCluster flink;
              • End diff –

          Thanks for telling me so useful message. Appreciate it.

          Show
          githubbot ASF GitHub Bot added a comment - Github user zhangminglei commented on a diff in the pull request: https://github.com/apache/flink/pull/3710#discussion_r111666373 — Diff: flink-clients/src/main/java/org/apache/flink/client/LocalExecutor.java — @@ -59,7 +59,7 @@ private final Object lock = new Object(); /** The mini cluster on which to execute the local programs */ private LocalFlinkMiniCluster flink; + private volatile LocalFlinkMiniCluster flink; End diff – Thanks for telling me so useful message. Appreciate it.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zhangminglei commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3710#discussion_r111666520

          — Diff: flink-clients/src/main/java/org/apache/flink/client/LocalExecutor.java —
          @@ -59,7 +59,7 @@
          private final Object lock = new Object();

          /** The mini cluster on which to execute the local programs */

          • private LocalFlinkMiniCluster flink;
            + private volatile LocalFlinkMiniCluster flink;
              • End diff –

          @zentol

          Show
          githubbot ASF GitHub Bot added a comment - Github user zhangminglei commented on a diff in the pull request: https://github.com/apache/flink/pull/3710#discussion_r111666520 — Diff: flink-clients/src/main/java/org/apache/flink/client/LocalExecutor.java — @@ -59,7 +59,7 @@ private final Object lock = new Object(); /** The mini cluster on which to execute the local programs */ private LocalFlinkMiniCluster flink; + private volatile LocalFlinkMiniCluster flink; End diff – @zentol
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zhangminglei commented on the issue:

          https://github.com/apache/flink/pull/3710

          @zentol I have updated the code. Thanks again.

          Show
          githubbot ASF GitHub Bot added a comment - Github user zhangminglei commented on the issue: https://github.com/apache/flink/pull/3710 @zentol I have updated the code. Thanks again.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zentol commented on the issue:

          https://github.com/apache/flink/pull/3710

          merging.

          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on the issue: https://github.com/apache/flink/pull/3710 merging.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zhangminglei commented on the issue:

          https://github.com/apache/flink/pull/3710

          @zentol I am appreciate it.

          Show
          githubbot ASF GitHub Bot added a comment - Github user zhangminglei commented on the issue: https://github.com/apache/flink/pull/3710 @zentol I am appreciate it.
          Hide
          Zentol Chesnay Schepler added a comment -

          283f5efd50bdb3e94cc947d1edab6fc0c8cbc77e

          Show
          Zentol Chesnay Schepler added a comment - 283f5efd50bdb3e94cc947d1edab6fc0c8cbc77e
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/flink/pull/3710

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/3710

            People

            • Assignee:
              mingleizhang mingleizhang
              Reporter:
              yuzhihong@gmail.com Ted Yu
            • Votes:
              1 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development