You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jclouds.apache.org by Anton Panasenko <no...@github.com> on 2013/06/14 13:46:32 UTC
[jclouds] Implemented JCLOUDS-127 (#32)
Added isOpen, isClosed method in Context, Closer class
You can merge this Pull Request by running:
git pull https://github.com/dieu/jclouds JCLOUDS-127
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/32
-- Commit Summary --
* Added isOpen, isClosed method in Context, Closer class
-- File Changes --
M core/src/main/java/org/jclouds/Context.java (10)
M core/src/main/java/org/jclouds/internal/ContextImpl.java (12)
M core/src/main/java/org/jclouds/lifecycle/Closer.java (31)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/32.patch
https://github.com/jclouds/jclouds/pull/32.diff
Re: [jclouds] JCLOUDS-127 (#32)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #456](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/456/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19578772
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Andrew Phillips <no...@github.com>.
> }
>
> + @Test
> + void testCloserCallOneClose() throws IOException, InterruptedException {
> + Injector i = createInjector();
> + final Closer closer = i.getInstance(Closer.class);
> + assert closer.getState() == Closer.State.OPEN;
> +
> + Closeable closeable = EasyMock.createMock(Closeable.class);
> +
> + closeable.close();
> +
> + EasyMock.expectLastCall().times(1);
> +
> + EasyMock.replay(closeable);
With some static imports, this could boil down to:
```
Closeable closeable = createStrictMock(Closeable.class);
closeable.close();
expectLastCall(); // don't need times(1), see "If no call count is specified, one call is expected." in http://www.easymock.org/EasyMock3_0_Documentation.html
replay(closeable);
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4723865
Re: [jclouds] JCLOUDS-127 (#32)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #91](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/91/) UNSTABLE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19485322
Re: [jclouds] JCLOUDS-127 (#32)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #439](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/439/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19454602
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Anton Panasenko <no...@github.com>.
> public void addToClose(Closeable toClose) {
> methodsToClose.add(toClose);
> }
>
> - public void close() throws IOException {
> + public synchronized void close() throws IOException {
> + this.isOpen = false;
yep, I think about this:
Since close can run for a long time, you can get a situation where isOpen true, but use context is impossible. What do you think about this situation?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4705219
Re: [jclouds] JCLOUDS-127 (#32)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #440](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/440/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19456757
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Zack Shoylev <no...@github.com>.
Synchronized method and an if statement also has the advantage of less code, or simpler code at least.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19482389
Re: [jclouds] JCLOUDS-127 (#32)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #105](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/105/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19578687
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Ignasi Barrera <no...@github.com>.
Also add an assertion to verify that, after closing the Closer, the value of the flag/state is the right one in the tests of the [LifeCycleModuleTest](https://github.com/jclouds/jclouds/blob/master/core/src/test/java/org/jclouds/lifecycle/config/LifeCycleModuleTest.java) class.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19454714
Re: [jclouds] JCLOUDS-127 (#32)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #88](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/88/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19468338
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Andrew Phillips <no...@github.com>.
> @@ -67,4 +67,8 @@
> @Override
> void close();
>
> + /**
> + * @return true if context open
> + */
> + boolean isOpen();
Ah, sorry, yes...I missed that. See comment below.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4734273
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Ignasi Barrera <no...@github.com>.
This looks good to me. As soon as you squash the commits I'll take the patch and merge the changes.
Thanks @demobox for the review and @dieu for all the fixes!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19576814
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Andrew Phillips <no...@github.com>.
> public void addToClose(Closeable toClose) {
> methodsToClose.add(toClose);
> }
>
> public void close() throws IOException {
> - Collections.reverse(methodsToClose);
> - for (Closeable toClose : methodsToClose) {
> - toClose.close();
> + if (state.compareAndSet(State.OPEN, State.CLOSING)) {
Tricky, I agree. I think after seeing the various review comments we can say that if it passes all the tests, we should be OK with this change. Once it's committed perhaps an email to the user@ list might not be a bad idea.
@nacx What do you think?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4723366
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Anton Panasenko <no...@github.com>.
> + void testCloserCallOneClose() throws IOException, InterruptedException {
> + Injector i = createInjector();
> + final Closer closer = i.getInstance(Closer.class);
> + assert closer.getState() == Closer.State.OPEN;
> +
> + Closeable closeable = EasyMock.createMock(Closeable.class);
> +
> + closeable.close();
> +
> + EasyMock.expectLastCall().times(1);
> +
> + EasyMock.replay(closeable);
> +
> + closer.addToClose(closeable);
> +
> + Runnable close = new Runnable() {
fixed
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4727381
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Anton Panasenko <no...@github.com>.
> }
>
> + @Test
> + void testCloserCallOneClose() throws IOException, InterruptedException {
> + Injector i = createInjector();
> + final Closer closer = i.getInstance(Closer.class);
> + assert closer.getState() == Closer.State.OPEN;
fixed
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4727366
Re: [jclouds] JCLOUDS-127 (#32)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #448](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/448/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19493476
Re: [jclouds] JCLOUDS-127 (#32)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #444](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/444/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19485415
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Anton Panasenko <no...@github.com>.
> @@ -35,14 +35,32 @@
> // guice is single threaded. no need to lock this
> List<Closeable> methodsToClose = Lists.<Closeable> newArrayList();
>
> + public enum State {
> + OPEN,
> + CLOSING,
> + CLOSED
> + }
> +
> + State state;
We discussed with @nacx synchronize close, and did not come to a consensus, I think that should be threadsafe. AtomicReference a good candidate:
```
this.state.compareAndSet(State.OPEN, State.CLOSING);
.....
this.state.set(State.CLOSED);
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4709929
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Andrew Phillips <no...@github.com>.
> @@ -72,7 +72,12 @@ public void close() {
> closeQuietly(closer);
> }
>
> - /**
> + @Override
> + public boolean isOpen() {
> + return closer.getState() == Closer.State.OPEN;
Formatting? Should be 3 spaces, I think.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4714159
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Andrew Phillips <no...@github.com>.
> +
> + closeable.close();
> +
> + EasyMock.expectLastCall().times(1);
> +
> + EasyMock.replay(closeable);
> +
> + closer.addToClose(closeable);
> +
> + Runnable close = new Runnable() {
> + @Override
> + public void run() {
> + try {
> + closer.close();
> + } catch (IOException e) {
> + assert false;
Add a message to the assert to explain the failure
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4723881
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Ignasi Barrera <no...@github.com>.
I'd change a few things in this commit:
* I'd add only one method to the interface: isOpen() or isClosed(), as one result implies the other.
* If you want to use the OPEN and CLOSED states instead of a boolean flag, use an enum instead of three ints.
* There is no need to lock the method: Just add a default constructor that sets the state to open, and change it to closed once the loop in the close() is done.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19454386
Re: [jclouds] JCLOUDS-127 (#32)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #95](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/95/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19493394
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Andrew Phillips <no...@github.com>.
> public void addToClose(Closeable toClose) {
> methodsToClose.add(toClose);
> }
>
> public void close() throws IOException {
> - Collections.reverse(methodsToClose);
> - for (Closeable toClose : methodsToClose) {
> - toClose.close();
> + if (state.compareAndSet(State.OPEN, State.CLOSING)) {
> Anyway, this PR looks good to me and if you @demobox agree, I think it can be merged once the isOpen() comment is addressed,
> and we can open the try/catch thing as a separate issue.
+1 for tackling the `try...catch` piece as a separate improvement.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4723964
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Ignasi Barrera <no...@github.com>.
@dieu, I agree with you: the close() method should better be thread-safe, and as Andrew pointed out, the state variable should also be private, to avoid unexpected internal state changes.
Using an AtomicReference or not, the important thing is to make sure that stuff is closed only once by only one thread. That means the loop should be protected so only the first thread enters it. The close method should be thread-safe, and something like:
if OPEN:
change to CLOSING
close stuff
change to CLOSED
Only the first thread should enter the condition and close stuff, and the other ones should just do nothing. That's why I asked about synchronizing the method (as it does nothing else). Atomically modifying the state variable might not be enough to protect the loop.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19481858
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Anton Panasenko <no...@github.com>.
@nacx Ok, I change, please review :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19465631
Re: [jclouds] JCLOUDS-127 (#32)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #449](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/449/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19511960
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Anton Panasenko <no...@github.com>.
> @@ -112,7 +120,86 @@ protected void configure() {
> });
> PostConstructable postConstructable = i.getInstance(PostConstructable.class);
> assert postConstructable.isStarted;
> + }
> +
> + @Test
> + void testCloserClosingState() throws InterruptedException {
> + Injector i = createInjector();
fixed
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4727363
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Anton Panasenko <no...@github.com>.
> @@ -35,14 +36,33 @@
> // guice is single threaded. no need to lock this
> List<Closeable> methodsToClose = Lists.<Closeable> newArrayList();
>
> + public enum State {
> + OPEN,
> + CLOSING,
> + CLOSED
> + }
> +
> + private final AtomicReference<State> state;
> +
> + public Closer() {
> + this.state = new AtomicReference<State>(State.OPEN);
fixed
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4727392
Re: [jclouds] JCLOUDS-127 (#32)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #106](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/106/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19580594
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Andrew Phillips <no...@github.com>.
> @@ -112,7 +120,86 @@ protected void configure() {
> });
> PostConstructable postConstructable = i.getInstance(PostConstructable.class);
> assert postConstructable.isStarted;
> + }
> +
> + @Test
> + void testCloserClosingState() throws InterruptedException {
> + Injector i = createInjector();
What is this test supposed to test? Simply the sequence of states or also some multi-threaded stuff? If so, what exactly? Also a [`CountDownLatch`](http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/CountDownLatch.html) is probably better suited to the handoff than an `AtomicBoolean` with empty loops.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4723775
Re: [jclouds] JCLOUDS-127 (#32)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #89](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/89/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19470672
Re: [jclouds] JCLOUDS-127 (#32)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #453](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/453/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19558491
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Andrew Phillips <no...@github.com>.
> @@ -89,9 +92,11 @@ void testCloserPreDestroyOrder() throws IOException {
> named(PROPERTY_IO_WORKER_THREADS)));
Do we need the additions here? This covers the same logic as `testCloserClosesExecutor` above, no?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4705064
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Ignasi Barrera <no...@github.com>.
Merged: https://git-wip-us.apache.org/repos/asf?p=incubator-jclouds.git;a=commit;h=ff3405c967b9fd6701fa39c6bb57d0308dd6e1e6
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19580913
Re: [jclouds] JCLOUDS-127 (#32)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #87](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/87/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19454877
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Anton Panasenko <no...@github.com>.
@nacx yes, thx :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19480231
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Anton Panasenko <no...@github.com>.
@nacx thx, I agree and understand.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19482391
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Ignasi Barrera <no...@github.com>.
Then, instead of having N methods returning a boolean I think it is more convenient to have only one returning the state. (Although IMHO, I don't see the point in explictly detecting a "closing" state, I think having the isOpen os isClose would be enough).
If there is more than one thread calling the close method, that means that N threads are sharing the same context object. It could be the responsibility of callers to properly synchronize those calls. Anyway, if it has to be made thread-safe, given that the closer is a singleton per context (and it should be only one), you could just declare the close method as synchronized. Since it is going to be invoked as the last operation, it should not be a problem to synchronize it.
Perhaps someone else have a string opinion on this (synchronizing stuff is always a conflictive thing :)).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19459935
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Andrew Phillips <no...@github.com>.
> public void addToClose(Closeable toClose) {
> methodsToClose.add(toClose);
> }
>
> public void close() throws IOException {
> - Collections.reverse(methodsToClose);
> - for (Closeable toClose : methodsToClose) {
> - toClose.close();
> + if (state.compareAndSet(State.OPEN, State.CLOSING)) {
This is now a change in logic...`close` will only be called **once** now. Previously, it was possible to call `close` multiple times, and the closer would run multiple times too.
I think the new logic is _nicer_ but I think we should try to verify whether this might impact users...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4714168
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Anton Panasenko <no...@github.com>.
> @@ -67,4 +67,8 @@
> @Override
> void close();
>
> + /**
> + * @return true if context open
> + */
> + boolean isOpen();
`State` relate to `Closer`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4727297
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Anton Panasenko <no...@github.com>.
> @@ -35,14 +36,33 @@
> // guice is single threaded. no need to lock this
> List<Closeable> methodsToClose = Lists.<Closeable> newArrayList();
>
> + public enum State {
> + OPEN,
> + CLOSING,
> + CLOSED
> + }
> +
> + private AtomicReference<State> state;
thx, fixed
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4715959
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Andrew Phillips <no...@github.com>.
Looks good to me! @nacx Anything to add?
@dieu If @nacx is good, could you finally just squash these down into one commit? Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19576598
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Andrew Phillips <no...@github.com>.
> @@ -35,14 +38,33 @@
> // guice is single threaded. no need to lock this
> List<Closeable> methodsToClose = Lists.<Closeable> newArrayList();
>
> + public enum State {
> + OPEN,
> + CLOSING,
> + CLOSED
> + }
@dieu As you correctly said above: `State` relates to the **closer**, not the **context**. In that case, would it make more sense if the states were called `AVAILABLE`, `PROCESSING` and `DONE`, or similar? Because the **closer** itself isn't really "open" or "closed", it's the **context** that is open, closing and closed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4734338
Re: [jclouds] JCLOUDS-127 (#32)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #96](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/96/) UNSTABLE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19511894
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Anton Panasenko <no...@github.com>.
> public void addToClose(Closeable toClose) {
> methodsToClose.add(toClose);
> }
>
> public void close() throws IOException {
> - Collections.reverse(methodsToClose);
> - for (Closeable toClose : methodsToClose) {
> - toClose.close();
> + if (state.compareAndSet(State.OPEN, State.CLOSING)) {
I agree. How can we test it?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4715961
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Andrew Phillips <no...@github.com>.
> @@ -35,14 +38,33 @@
> // guice is single threaded. no need to lock this
> List<Closeable> methodsToClose = Lists.<Closeable> newArrayList();
>
> + public enum State {
> + OPEN,
> + CLOSING,
> + CLOSED
> + }
PS: I think this will be my last comment/point to address...thanks for bearing with me! ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4734576
Re: [jclouds] JCLOUDS-127 (#32)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #443](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/443/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19473826
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Andrew Phillips <no...@github.com>.
> @@ -35,14 +36,33 @@
> // guice is single threaded. no need to lock this
> List<Closeable> methodsToClose = Lists.<Closeable> newArrayList();
>
> + public enum State {
> + OPEN,
> + CLOSING,
> + CLOSED
> + }
> +
> + private AtomicReference<State> state;
Make `final`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4714162
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Andrew Phillips <no...@github.com>.
> @@ -35,14 +36,25 @@
> // guice is single threaded. no need to lock this
> List<Closeable> methodsToClose = Lists.<Closeable> newArrayList();
>
> + volatile boolean isOpen;
`open` rather than `isOpen`? Matches bean naming conventions. And how about an AtomicBoolean rather than `volatile`? Then we don't need to synchronize the `close` method...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4705028
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Anton Panasenko <no...@github.com>.
thanks Ignasi,
* As I understand it, close can long be performed. According to this interesting to know that the context is not open when started closing. And I think the two methods are needed.
* Ok.
* Close can called from only one thread?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19457108
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Anton Panasenko <no...@github.com>.
@nacx and @demobox thx.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19576946
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Anton Panasenko <no...@github.com>.
> +
> + closeable.close();
> +
> + EasyMock.expectLastCall().times(1);
> +
> + EasyMock.replay(closeable);
> +
> + closer.addToClose(closeable);
> +
> + Runnable close = new Runnable() {
> + @Override
> + public void run() {
> + try {
> + closer.close();
> + } catch (IOException e) {
> + assert false;
fixed
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4727379
Re: [jclouds] JCLOUDS-127 (#32)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #450](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/450/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19512491
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Anton Panasenko <no...@github.com>.
> @@ -89,9 +92,11 @@ void testCloserPreDestroyOrder() throws IOException {
> named(PROPERTY_IO_WORKER_THREADS)));
yep, `testCloserPreDestroyOrder` and `testCloserClosesExecutor` very similar.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4705158
Re: [jclouds] JCLOUDS-127 (#32)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #441](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/441/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19468618
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Anton Panasenko <no...@github.com>.
@nacx and @demobox What do you think about stateflow?
```
OPEN -> CLOSING -> CLOSED
```
but is it possible?
```
CLOSED -> CLOSING -> CLOSED
```
and accordingly call to N times the method close?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19481685
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Ignasi Barrera <no...@github.com>.
We're getting there :) But I think there is still a test missing that asserts that the Closer is properly changed to CLOSING state.
You could add a new test method in the class, that adds a custom Closeable to the closer, and calls the close() in a separate thread. This way you can make your custom closeable block until you have properly asserted the CLOSING state, and then continue and assert the CLOSED one. Makes sense?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19469548
Re: [jclouds] JCLOUDS-127 (#32)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #101](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/101/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19557842
Re: [jclouds] Implemented JCLOUDS-127 (#32)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #86](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/86/) FAILURE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19452597
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Anton Panasenko <no...@github.com>.
> }
>
> + @Test
> + void testCloserCallOneClose() throws IOException, InterruptedException {
> + Injector i = createInjector();
> + final Closer closer = i.getInstance(Closer.class);
> + assert closer.getState() == Closer.State.OPEN;
> +
> + Closeable closeable = EasyMock.createMock(Closeable.class);
> +
> + closeable.close();
> +
> + EasyMock.expectLastCall().times(1);
> +
> + EasyMock.replay(closeable);
fixed
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4727373
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Andrew Phillips <no...@github.com>.
> @@ -67,4 +67,8 @@
> @Override
> void close();
>
> + /**
> + * @return true if context open
> + */
> + boolean isOpen();
If `Context.State` is public should we not have that enum _here_ and then have a `State getState();` method instead? Now we can check for one of three possible states at the interface level, but if we cast to the implementation we can get all _three_ possible states.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4723536
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Anton Panasenko <no...@github.com>.
> @@ -35,14 +36,25 @@
> // guice is single threaded. no need to lock this
> List<Closeable> methodsToClose = Lists.<Closeable> newArrayList();
>
> + volatile boolean isOpen;
yep, I remove synchronize primitives, until there is a solution in the mailing list.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4705194
Re: [jclouds] JCLOUDS-127 (#32)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #442](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/442/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19471024
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Andrew Phillips <no...@github.com>.
> @@ -72,7 +72,12 @@ public void close() {
> closeQuietly(closer);
> }
>
> - /**
> + @Override
> + public boolean isOpen() {
> + return closer.getState() == Closer.State.OPEN;
Static import `Closer.State.*`? Then this can just be
```
return closer.getState() == OPEN;
```
which reads a little easier?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4714164
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Andrew Phillips <no...@github.com>.
> public void addToClose(Closeable toClose) {
> methodsToClose.add(toClose);
> }
>
> - public void close() throws IOException {
> + public synchronized void close() throws IOException {
> + this.isOpen = false;
One consequence of this is that `isOpen` will be false even **before** all the Closeables have been closed. I assume this is by design? In any case, it's not covered by a test case.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4705081
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Ignasi Barrera <no...@github.com>.
> public void addToClose(Closeable toClose) {
> methodsToClose.add(toClose);
> }
>
> public void close() throws IOException {
> - Collections.reverse(methodsToClose);
> - for (Closeable toClose : methodsToClose) {
> - toClose.close();
> + if (state.compareAndSet(State.OPEN, State.CLOSING)) {
IMO this change is OK. I can't come up with a use case where closing the context multiple times would be needed. The main things to be closed in the context are the executor services, and once they are shutdown I don't see the point in shutting down them again.
Also, once a context is closed, there are no means to "reopen" it, apart from creating a new one, so I think this won't break any existing code (unless someone is adding arbitrary stuff to the Closer once the context is created, which I doubt).
Having said this, would it be a good idea to try/catch each call to `toClose.close()` so even if a Closebale can't be closed we try to close the others? Closing the context should be the very last operation when using jclouds. I think it should be good to try to close everything and just leave a log with the close failures.
Anyway, this PR looks good to me and if you @demobox agree, I think it can be merged once the `isOpen()` comment is addressed, and we can open the try/catch thing as a separate issue.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4723718
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Andrew Phillips <no...@github.com>.
> + void testCloserCallOneClose() throws IOException, InterruptedException {
> + Injector i = createInjector();
> + final Closer closer = i.getInstance(Closer.class);
> + assert closer.getState() == Closer.State.OPEN;
> +
> + Closeable closeable = EasyMock.createMock(Closeable.class);
> +
> + closeable.close();
> +
> + EasyMock.expectLastCall().times(1);
> +
> + EasyMock.replay(closeable);
> +
> + closer.addToClose(closeable);
> +
> + Runnable close = new Runnable() {
Perhaps call it something like `closeContext`..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4723943
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Andrew Phillips <no...@github.com>.
> }
>
> + @Test
> + void testCloserCallOneClose() throws IOException, InterruptedException {
> + Injector i = createInjector();
> + final Closer closer = i.getInstance(Closer.class);
> + assert closer.getState() == Closer.State.OPEN;
This `assert` is not necessary, other tests already verify that the closer starts in state `OPEN`. And I don't think the final is needed either.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4723801
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Andrew Phillips <no...@github.com>.
> @@ -35,14 +36,33 @@
> // guice is single threaded. no need to lock this
> List<Closeable> methodsToClose = Lists.<Closeable> newArrayList();
>
> + public enum State {
> + OPEN,
> + CLOSING,
> + CLOSED
> + }
> +
> + private final AtomicReference<State> state;
> +
> + public Closer() {
> + this.state = new AtomicReference<State>(State.OPEN);
Small style comment: perhaps also import `State.*` here, if that's possible (perhaps not, since it's declared in this class)?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4723349
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Anton Panasenko <no...@github.com>.
> @@ -35,14 +38,33 @@
> // guice is single threaded. no need to lock this
> List<Closeable> methodsToClose = Lists.<Closeable> newArrayList();
>
> + public enum State {
> + OPEN,
> + CLOSING,
> + CLOSED
> + }
thx, fixed :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4735362
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Ignasi Barrera <no...@github.com>.
Closed #32.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Andrew Phillips <no...@github.com>.
> @@ -35,14 +35,32 @@
> // guice is single threaded. no need to lock this
> List<Closeable> methodsToClose = Lists.<Closeable> newArrayList();
>
> + public enum State {
> + OPEN,
> + CLOSING,
> + CLOSED
> + }
> +
> + State state;
We're now in a possible race condition here. Is there any reason this field isn't private, by the way? Do we need an AtomicReference here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4705782
Re: [jclouds] JCLOUDS-127 (#32)
Posted by Anton Panasenko <no...@github.com>.
> @@ -72,7 +72,12 @@ public void close() {
> closeQuietly(closer);
> }
>
> - /**
> + @Override
> + public boolean isOpen() {
> + return closer.getState() == Closer.State.OPEN;
thks, fixed
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32/files#r4715958
Re: [jclouds] JCLOUDS-127 (#32)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #97](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/97/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19512396
Re: [jclouds] JCLOUDS-127 (#32)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #90](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/90/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/32#issuecomment-19473328