You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by neykov <gi...@git.apache.org> on 2015/07/27 16:15:06 UTC

[GitHub] incubator-brooklyn pull request: Don't leak machines on shutdown

GitHub user neykov opened a pull request:

    https://github.com/apache/incubator-brooklyn/pull/771

    Don't leak machines on shutdown

    STOPping an app and then doing a shutdown-and-stop-apps could leak machines because the second stop (from the shutdown) can complete faster as it's not re-releasing the machines, then unmanage the app and cancel the first stop's tasks. Cancelling the tasks will not release the machines properly.
    
    This PR works around the problem in the shutdown code. Double stopping an app will still keep the same behaviour.

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

    $ git pull https://github.com/neykov/incubator-brooklyn wait-apps-shutdown

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

    https://github.com/apache/incubator-brooklyn/pull/771.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 #771
    
----
commit b93c2995f5212c3680df19c90961d242088d3429
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date:   2015-07-23T15:54:30Z

    Graceful exit on REST shutdown call
    
    Instead of calling System.exit, exit the main thread.

commit cad297b318d7f063ade3930e77b1e56e79d50e16
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date:   2015-07-24T20:12:22Z

    Wait apps to stop before shutting down.

commit ce2fd3f29dd639104c1128ba80279478c380b78c
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date:   2015-07-27T13:28:25Z

    Add tests for apps double stop

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Don't leak machines on shutdown

Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/771#discussion_r35760476
  
    --- Diff: usage/rest-server/src/test/java/brooklyn/rest/BrooklynRestApiLauncher.java ---
    @@ -235,6 +238,7 @@ private ContextHandler servletContextHandler(ManagementContext managementContext
             ResourceConfig config = new DefaultResourceConfig();
             for (Object r: BrooklynRestApi.getAllResources())
                 config.getSingletons().add(r);
    +        config.getSingletons().add(new ShutdownRequestListenerProvider(new TestShutdownRequestListener()));
    --- End diff --
    
    Update code to use the field. The field itself is not used elsewhere, but added it anyway for consistency.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Don't leak machines on shutdown

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:

    https://github.com/apache/incubator-brooklyn/pull/771#issuecomment-125541071
  
    Thanks @neykov - looks really good!
    
    I've added a bunch of minor comments. Please ping me when you've been through those, and incorporated those that you think should be addressed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Don't leak machines on shutdown

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/771#discussion_r35632009
  
    --- Diff: usage/cli/src/main/java/brooklyn/cli/Main.java ---
    @@ -964,4 +979,35 @@ public ToStringHelper string() {
         protected Class<? extends BrooklynCommand> cliDefaultInfoCommand() {
             return DefaultInfoCommand.class;
         }
    +    
    +    public static class AppShutdownRequestListener implements ShutdownRequestListener {
    +        private AtomicBoolean mutex = new AtomicBoolean();
    +
    +        @Override
    +        public void onShutdownRequest() {
    +            synchronized (mutex) {
    --- End diff --
    
    I prefer use of CountDownLatch - slightly simpler code, which is always good when dealing with multi-threading.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Don't leak machines on shutdown

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/771#discussion_r35632448
  
    --- Diff: usage/rest-server/src/main/java/brooklyn/rest/resources/ServerResource.java ---
    @@ -171,17 +207,22 @@ public void run() {
                         hasAppErrorsOrTimeout.set(true);
                         
                         if (!terminateTried) {
    -                        ((ManagementContextInternal)mgmt()).terminate(); 
    +                        ((ManagementContextInternal)mgmt).terminate(); 
                         }
                     } finally {
     
                         complete();
                     
                         if (!hasAppErrorsOrTimeout.get() || forceShutdownOnError) {
    -                        //give the http request a chance to complete gracefully
    +                        //give the http request a chance to complete gracefully, the server will be stopped in a shutdown hook
                             Time.sleep(delayForHttpReturn);
    -                        
    -                        System.exit(0);
    +
    +                        if (shutdownRequestListener != null) {
    +                            shutdownRequestListener.onShutdownRequest();
    +                        } else {
    +                            log.warn("ShutdownRequestListener not set, existing process");
    --- End diff --
    
    Should be "exiting process", instead of "existing process"?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Don't leak machines on shutdown

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/771#discussion_r35632519
  
    --- Diff: usage/rest-server/src/main/java/brooklyn/rest/util/ShutdownRequestListener.java ---
    @@ -0,0 +1,23 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.rest.util;
    +
    +public interface ShutdownRequestListener {
    --- End diff --
    
    Should this be called "listener" or "handler"?
    
    I see in `ServerResource.java` that it does not do `System.exit(0)` if there is a listener set, so it feels like it is not just a listener.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Don't leak machines on shutdown

Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/771#discussion_r35760168
  
    --- Diff: software/base/src/test/java/brooklyn/entity/basic/SoftwareProcessEntityTest.java ---
    @@ -568,4 +706,85 @@ protected String getInstallLabelExtraSalt() {
                 return (String)getEntity().getConfigRaw(ConfigKeys.newStringConfigKey("salt"), true).or((String)null);
             }
         }
    +
    +    public static class ReleaseLatchLocation extends SimulatedLocation {
    +        private static final long serialVersionUID = 1L;
    +        
    +        private AtomicBoolean lock = new AtomicBoolean();
    +        private boolean isBlocked;
    +
    +        public void unblock() {
    +            synchronized (lock) {
    +                lock.set(true);
    +                lock.notifyAll();
    +            }
    +        }
    +        @Override
    +        public void release(MachineLocation machine) {
    +            super.release(machine);
    +            synchronized (lock) {
    +                if (!lock.get()) {
    +                    try {
    +                        isBlocked = true;
    +                        lock.wait();
    +                        isBlocked = false;
    +                    } catch (InterruptedException e) {
    +                        throw Exceptions.propagate(e);
    +                    }
    +                }
    +            }
    +        }
    +
    +        public boolean isBlocked() {
    +            return isBlocked;
    +        }
    +
    +    }
    +
    +    public static class GenericSoftwareProcessTestDriver implements SoftwareProcessDriver {
    --- End diff --
    
    Agree that some code deleting is in order. In this case I explicitly didn't want an SshDriver implementation though (as most of the above are).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Don't leak machines on shutdown

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/771#discussion_r35631925
  
    --- Diff: software/base/src/test/java/brooklyn/entity/basic/SoftwareProcessEntityTest.java ---
    @@ -568,4 +706,85 @@ protected String getInstallLabelExtraSalt() {
                 return (String)getEntity().getConfigRaw(ConfigKeys.newStringConfigKey("salt"), true).or((String)null);
             }
         }
    +
    +    public static class ReleaseLatchLocation extends SimulatedLocation {
    +        private static final long serialVersionUID = 1L;
    +        
    +        private AtomicBoolean lock = new AtomicBoolean();
    +        private boolean isBlocked;
    +
    +        public void unblock() {
    +            synchronized (lock) {
    --- End diff --
    
    Given it's for a test, I don't really care but...
    
    I much prefer using higher-level synchronization utils from java.util.concurrent whenever possible, rather than synchronize.
    An object.wait() can wake up prematurely, which is why one should always wrap it in a while block rather than an if block.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Don't leak machines on shutdown

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/771#discussion_r35632364
  
    --- Diff: usage/launcher/src/main/java/brooklyn/launcher/BrooklynLauncher.java ---
    @@ -772,6 +782,7 @@ protected void startWebApps() {
                 webServer.setPublicAddress(publicAddress);
                 if (port!=null) webServer.setPort(port);
                 if (useHttps!=null) webServer.setHttpsEnabled(useHttps);
    +            webServer.setShutdownRequestListener(shutdownRequestListener);
    --- End diff --
    
    I was initilly a little worried about nulls being passed through here (but looks like it's ok at the end, where it's finally used).
    
    If nulls are allowed, then worth including `@Nullable` on the parameter.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Don't leak machines on shutdown

Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/771#discussion_r35760368
  
    --- Diff: usage/rest-server/src/main/java/brooklyn/rest/resources/ServerResource.java ---
    @@ -191,6 +232,15 @@ public void run() {
                     }
                 }
     
    +            private boolean hasStoppingApps(ManagementContext mgmt) {
    --- End diff --
    
    All apps are stopping by now, but you are right the suggested name will better reflect the code in it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Don't leak machines on shutdown

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/771#discussion_r35632056
  
    --- Diff: usage/cli/src/main/java/brooklyn/cli/Main.java ---
    @@ -964,4 +979,35 @@ public ToStringHelper string() {
         protected Class<? extends BrooklynCommand> cliDefaultInfoCommand() {
             return DefaultInfoCommand.class;
         }
    +    
    +    public static class AppShutdownRequestListener implements ShutdownRequestListener {
    +        private AtomicBoolean mutex = new AtomicBoolean();
    +
    +        @Override
    +        public void onShutdownRequest() {
    +            synchronized (mutex) {
    --- End diff --
    
    FYI (completely different topic really!) I'm toying with a new heuristic that use of "synchronized" should be considered a warning sign in most code; it could use things like `CountDownLatch`, `ConcurrentHashMap`, etc; or one could extract the concurrency-handling logic into a wrapping class or into a different threading model (e.g. actor-based, or "channels").
    
    I like Go's slogan: "Do not communicate by sharing memory; instead, share memory by communicating."
    
    Obviously `synchronized` is the right thing to do sometimes, but it's used far too often.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Don't leak machines on shutdown

Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:

    https://github.com/apache/incubator-brooklyn/pull/771#issuecomment-125961873
  
    Thanks for the great comments @aledsage, should all be addressed now.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Don't leak machines on shutdown

Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/771#discussion_r35760187
  
    --- Diff: usage/cli/src/main/java/brooklyn/cli/Main.java ---
    @@ -964,4 +979,35 @@ public ToStringHelper string() {
         protected Class<? extends BrooklynCommand> cliDefaultInfoCommand() {
             return DefaultInfoCommand.class;
         }
    +    
    +    public static class AppShutdownRequestListener implements ShutdownRequestListener {
    +        private AtomicBoolean mutex = new AtomicBoolean();
    +
    +        @Override
    +        public void onShutdownRequest() {
    +            synchronized (mutex) {
    --- End diff --
    
    Agree, updated the code.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Don't leak machines on shutdown

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/771#discussion_r35631976
  
    --- Diff: software/base/src/test/java/brooklyn/entity/basic/SoftwareProcessEntityTest.java ---
    @@ -568,4 +706,85 @@ protected String getInstallLabelExtraSalt() {
                 return (String)getEntity().getConfigRaw(ConfigKeys.newStringConfigKey("salt"), true).or((String)null);
             }
         }
    +
    +    public static class ReleaseLatchLocation extends SimulatedLocation {
    +        private static final long serialVersionUID = 1L;
    +        
    +        private AtomicBoolean lock = new AtomicBoolean();
    +        private boolean isBlocked;
    +
    +        public void unblock() {
    +            synchronized (lock) {
    +                lock.set(true);
    +                lock.notifyAll();
    +            }
    +        }
    +        @Override
    +        public void release(MachineLocation machine) {
    +            super.release(machine);
    +            synchronized (lock) {
    +                if (!lock.get()) {
    +                    try {
    +                        isBlocked = true;
    +                        lock.wait();
    +                        isBlocked = false;
    +                    } catch (InterruptedException e) {
    +                        throw Exceptions.propagate(e);
    +                    }
    +                }
    +            }
    +        }
    +
    +        public boolean isBlocked() {
    +            return isBlocked;
    +        }
    +
    +    }
    +
    +    public static class GenericSoftwareProcessTestDriver implements SoftwareProcessDriver {
    --- End diff --
    
    For the future (not this PR)...
    Feels like this is useful for more than just here. For example, we already have:
    * `DoNothingSoftwareProcessDriver`
    * `SoftwareProcessEntityTest.SimulatedDriver`
    * `SoftwareProcessSshDriverIntegrationTest.SimulatedDriver`
    * `MockSshDriver` (which is a stub rather than mock)
    * `RestMockSimpleEntity.MockSshDriver`



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Don't leak machines on shutdown

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/771#discussion_r35632569
  
    --- Diff: usage/rest-server/src/main/java/brooklyn/rest/resources/ServerResource.java ---
    @@ -191,6 +232,15 @@ public void run() {
                     }
                 }
     
    +            private boolean hasStoppingApps(ManagementContext mgmt) {
    --- End diff --
    
    Should this be called `hasStoppableApps`?



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Don't leak machines on shutdown

Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/771#discussion_r35760288
  
    --- Diff: usage/launcher/src/main/java/brooklyn/launcher/BrooklynLauncher.java ---
    @@ -772,6 +782,7 @@ protected void startWebApps() {
                 webServer.setPublicAddress(publicAddress);
                 if (port!=null) webServer.setPort(port);
                 if (useHttps!=null) webServer.setHttpsEnabled(useHttps);
    +            webServer.setShutdownRequestListener(shutdownRequestListener);
    --- End diff --
    
    It's a simple setter which for me assumes `@Nullable`. I've already marked the provider constructor. Added the annotation here as well though, can't hurt.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Don't leak machines on shutdown

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:

    https://github.com/apache/incubator-brooklyn/pull/771#issuecomment-125991449
  
    Great, thanks @neykov - merging now.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Don't leak machines on shutdown

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/771#discussion_r35632729
  
    --- Diff: usage/rest-server/src/test/java/brooklyn/rest/BrooklynRestApiLauncher.java ---
    @@ -235,6 +238,7 @@ private ContextHandler servletContextHandler(ManagementContext managementContext
             ResourceConfig config = new DefaultResourceConfig();
             for (Object r: BrooklynRestApi.getAllResources())
                 config.getSingletons().add(r);
    +        config.getSingletons().add(new ShutdownRequestListenerProvider(new TestShutdownRequestListener()));
    --- End diff --
    
    How does this `new TestShutdownRequestListener` related to the `shutdownListener` field? Does anything use the field?



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Don't leak machines on shutdown

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/771#discussion_r35632768
  
    --- Diff: usage/rest-server/src/test/java/brooklyn/rest/resources/ServerResourceTest.java ---
    @@ -134,4 +131,52 @@ void testGetMissingConfigThrowsException() throws Exception {
                 }
             }
         }
    +
    +    // Alternatively could reuse a blocking location, see brooklyn.entity.basic.SoftwareProcessEntityTest.ReleaseLatchLocation
    +    @ImplementedBy(StopLatchEntityImpl.class)
    +    public interface StopLatchEntity extends EmptySoftwareProcess {
    +        public void unblock();
    +        public boolean isBlocked();
    +    }
    +
    +    public static class StopLatchEntityImpl extends EmptySoftwareProcessImpl implements StopLatchEntity {
    +        private AtomicBoolean lock = new AtomicBoolean();
    +        private boolean isBlocked;
    +
    +        @Override
    +        public void unblock() {
    +            synchronized (lock) {
    +                lock.set(true);
    +                lock.notifyAll();
    +            }
    +        }
    +
    +        @Override
    +        protected void postStop() {
    +            super.preStop();
    +            synchronized (lock) {
    +                if (!lock.get()) {
    --- End diff --
    
    See previous comment in ReleaseLatchLocation.release about `synchronized`. Could change to `CountDownLatch`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Don't leak machines on shutdown

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/incubator-brooklyn/pull/771


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Don't leak machines on shutdown

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/771#discussion_r35632819
  
    --- Diff: usage/rest-server/src/test/java/brooklyn/rest/util/TestShutdownRequestListener.java ---
    @@ -0,0 +1,39 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.rest.util;
    +
    +import brooklyn.rest.util.ShutdownRequestListener;
    +
    +public class TestShutdownRequestListener implements ShutdownRequestListener {
    +    boolean isRequested;
    --- End diff --
    
    Should declare this volatile, given it is multi-threaded usage.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---