Skip to content

Commit 5043783

Browse files
author
Sumedh Wale
committed
Use timed Process.waitFor instead of loop on exitValue()
- Process.exitValue() is unreliable and may return back an invalid value of -1 (as seen in current Oracle Java 1.8.0_321 release) instead of throwing an exception, so change all instances of looping on exitValue() to looping on timed waitFor or single waitFor instance - All test code changed as per above (except for derby internal tests etc which are never run in SnappyData) - All relevant product code changed as per above too - Other minor fixes to sporadic test failures
1 parent 310310e commit 5043783

25 files changed

Lines changed: 276 additions & 288 deletions

File tree

‎build.gradle‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -491,7 +491,7 @@ subprojects {
491491

492492
task wanTest(type:Test) {
493493
dependsOn "${subprojectBase}storeProduct"
494-
maxParallelForks = Math.sqrt(maxWorkers + 1) as int
494+
maxParallelForks = Math.sqrt(maxWorkers / 2 + 1) as int
495495
minHeapSize = '1g'
496496
maxHeapSize = '1g'
497497

‎gemfire-core/src/main/java/com/gemstone/gemfire/internal/OSProcess.java‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
import java.util.Iterator;
3737
import java.util.Map;
3838
import java.util.Set;
39+
import java.util.concurrent.TimeUnit;
3940
import java.util.zip.GZIPOutputStream;
4041

4142
import com.gemstone.gemfire.LogWriter;
@@ -350,16 +351,9 @@ public static int bgexec(String cmdarray[],
350351
try { process.getErrorStream().close(); } catch(IOException ignore){}
351352
try {
352353
// short count = 1000;
353-
boolean processIsStillRunning = true;
354-
while(processIsStillRunning) {
355-
Thread.sleep(10);
356-
try {
357-
process.exitValue();
358-
processIsStillRunning = false;
359-
} catch(IllegalThreadStateException itse) {
360-
// Ignore this, we are polling the exitStatus
361-
// instead of using the blocking Process#waitFor()
362-
}
354+
while (!process.waitFor(100, TimeUnit.MILLISECONDS)) {
355+
// Nothing to do, we are polling using timed Process#waitFor()
356+
// instead of using the blocking Process#waitFor()
363357
}
364358
} catch(InterruptedException ie) {
365359
Thread.currentThread().interrupt();

‎gemfire-core/src/main/java/com/gemstone/gemfire/internal/ProcessOutputReader.java‎

Lines changed: 122 additions & 97 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,19 @@
1414
* permissions and limitations under the License. See accompanying
1515
* LICENSE file.
1616
*/
17-
17+
1818
package com.gemstone.gemfire.internal;
1919

20-
import java.io.*;
21-
import java.util.*;
20+
import java.io.BufferedReader;
21+
import java.io.InputStream;
22+
import java.io.InputStreamReader;
23+
import java.io.PrintWriter;
24+
import java.util.ArrayList;
25+
import java.util.Collections;
26+
import java.util.List;
27+
import java.util.concurrent.TimeUnit;
2228

29+
import com.gemstone.gemfire.distributed.internal.InternalDistributedSystem;
2330
import com.gemstone.gemfire.internal.i18n.LocalizedStrings;
2431

2532
/**
@@ -29,107 +36,125 @@
2936
* read and the process has exited.
3037
*
3138
* @author darrel
32-
*
3339
*/
3440
public class ProcessOutputReader {
35-
private int exitCode;
36-
private String output;
37-
/**
38-
* Creates a process output reader for the given process.
39-
* @param p the process whose output should be read.
40-
*/
41-
public ProcessOutputReader(final Process p) {
42-
final List lines = Collections.synchronizedList(new ArrayList());
41+
private int exitCode;
42+
private final String output;
43+
44+
/**
45+
* Creates a process output reader for the given process.
46+
*
47+
* @param p the process whose output should be read.
48+
*/
49+
public ProcessOutputReader(final Process p) {
50+
final List<String> lines = Collections.synchronizedList(new ArrayList<>());
4351

44-
class ProcessStreamReader extends Thread {
45-
private BufferedReader reader;
46-
public int linecount = 0;
47-
public ProcessStreamReader(InputStream stream) {
48-
reader = new BufferedReader(new InputStreamReader(stream));
49-
}
50-
@Override
51-
public void run() {
52-
try {
53-
String line;
54-
while ((line = reader.readLine()) != null) {
55-
linecount++;
56-
lines.add(line);
57-
}
58-
reader.close();
59-
} catch (Exception e) {
60-
}
61-
}
62-
};
52+
class ProcessStreamReader extends Thread {
53+
private final BufferedReader reader;
54+
public int linecount = 0;
6355

64-
ProcessStreamReader stdout =
65-
new ProcessStreamReader(p.getInputStream());
66-
ProcessStreamReader stderr =
67-
new ProcessStreamReader(p.getErrorStream());
68-
stdout.start();
69-
stderr.start();
70-
try {stderr.join();} catch (InterruptedException ignore) {Thread.currentThread().interrupt();}
71-
try {stdout.join();} catch (InterruptedException ignore) {Thread.currentThread().interrupt();}
72-
this.exitCode = 0;
73-
int retryCount = 9;
74-
while (retryCount > 0) {
75-
retryCount--;
76-
try {
77-
exitCode = p.exitValue();
78-
break;
79-
} catch (IllegalThreadStateException e) {
80-
// due to bugs in Process we may not be able to get
81-
// a process's exit value.
82-
// We can't use Process.waitFor() because it can hang forever
83-
if (retryCount == 0) {
84-
if (stderr.linecount > 0) {
85-
// The process wrote to stderr so manufacture
86-
// an error exist code
87-
lines.add(LocalizedStrings.ProcessOutputReader_FAILED_TO_GET_EXIT_STATUS_AND_IT_WROTE_TO_STDERR_SO_SETTING_EXIT_STATUS_TO_1.toLocalizedString());
88-
exitCode = 1;
89-
}
90-
} else {
91-
// We need to wait around to give a chance for
92-
// the child to be reaped.See bug 19682
93-
try {
94-
Thread.sleep(1000);
95-
}
96-
catch (InterruptedException ignore) {
97-
Thread.currentThread().interrupt();
98-
// TODO a cancellation check here?
99-
}
100-
}
101-
}
102-
}
56+
public ProcessStreamReader(InputStream stream) {
57+
reader = new BufferedReader(new InputStreamReader(stream));
58+
}
10359

104-
java.io.StringWriter sw = new java.io.StringWriter();
105-
PrintWriter pw = new PrintWriter(sw);
106-
Iterator it = lines.iterator();
107-
while (it.hasNext()) {
108-
pw.println((String)it.next());
109-
}
110-
pw.close();
111-
try {
112-
sw.close();
113-
} catch (java.io.IOException ignore) {}
114-
StringBuffer buf = sw.getBuffer();
115-
if (buf != null && buf.length() > 0) {
116-
this.output = sw.toString();
117-
} else {
118-
this.output = "";
119-
}
60+
@Override
61+
public void run() {
62+
try {
63+
String line;
64+
while ((line = reader.readLine()) != null) {
65+
linecount++;
66+
lines.add(line);
67+
}
68+
reader.close();
69+
} catch (Exception ignore) {
70+
}
71+
}
12072
}
12173

122-
/**
123-
* Gets the process's exit status code. A code equal to 0 indicates
124-
* all is well.
125-
*/
126-
public int getExitCode() {
127-
return exitCode;
74+
ProcessStreamReader stdout = new ProcessStreamReader(p.getInputStream());
75+
ProcessStreamReader stderr = new ProcessStreamReader(p.getErrorStream());
76+
stdout.start();
77+
stderr.start();
78+
try {
79+
stderr.join();
80+
} catch (InterruptedException ignore) {
81+
Thread.currentThread().interrupt();
12882
}
129-
/**
130-
* Gets everything the process wrote to both stdout and stderr.
131-
*/
132-
public String getOutput() {
133-
return output;
83+
try {
84+
stdout.join();
85+
} catch (InterruptedException ignore) {
86+
Thread.currentThread().interrupt();
87+
}
88+
InterruptedException ie = null;
89+
this.exitCode = 0;
90+
int retryCount = 9;
91+
while (retryCount > 0) {
92+
retryCount--;
93+
try {
94+
// We need to wait around to give a chance for
95+
// the child to be reaped.See bug 19682
96+
if (p.waitFor(1000, TimeUnit.MILLISECONDS)) {
97+
exitCode = p.exitValue();
98+
break;
99+
}
100+
// due to bugs in Process we may not be able to get
101+
// a process's exit value.
102+
// We can't use Process.waitFor() because it can hang forever
103+
if (retryCount == 0) {
104+
if (stderr.linecount > 0) {
105+
// The process wrote to stderr so manufacture
106+
// an error exist code
107+
lines.add(LocalizedStrings.
108+
ProcessOutputReader_FAILED_TO_GET_EXIT_STATUS_AND_IT_WROTE_TO_STDERR_SO_SETTING_EXIT_STATUS_TO_1.
109+
toLocalizedString());
110+
exitCode = 1;
111+
}
112+
}
113+
} catch (InterruptedException e) {
114+
Thread.currentThread().interrupt();
115+
ie = e;
116+
// end the loop faster
117+
retryCount--;
118+
}
119+
}
120+
121+
java.io.StringWriter sw = new java.io.StringWriter();
122+
PrintWriter pw = new PrintWriter(sw);
123+
for (String line : lines) {
124+
pw.println(line);
134125
}
126+
pw.close();
127+
try {
128+
sw.close();
129+
} catch (java.io.IOException ignore) {
130+
}
131+
StringBuffer buf = sw.getBuffer();
132+
if (buf != null && buf.length() > 0) {
133+
this.output = sw.toString();
134+
} else {
135+
this.output = "";
136+
}
137+
if (ie != null) {
138+
final InternalDistributedSystem ds =
139+
InternalDistributedSystem.getConnectedInstance();
140+
if (ds != null) {
141+
ds.getCancelCriterion().checkCancelInProgress(ie);
142+
}
143+
}
144+
}
145+
146+
/**
147+
* Gets the process's exit status code. A code equal to 0 indicates
148+
* all is well.
149+
*/
150+
public int getExitCode() {
151+
return exitCode;
152+
}
153+
154+
/**
155+
* Gets everything the process wrote to both stdout and stderr.
156+
*/
157+
public String getOutput() {
158+
return output;
159+
}
135160
}

‎gemfire-core/src/main/java/com/gemstone/gemfire/management/internal/cli/commands/DiskStoreCommands.java‎

Lines changed: 15 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -23,16 +23,9 @@
2323
import java.io.InputStream;
2424
import java.io.InputStreamReader;
2525
import java.io.PrintStream;
26-
import java.util.ArrayList;
27-
import java.util.Arrays;
28-
import java.util.Collections;
29-
import java.util.HashMap;
30-
import java.util.HashSet;
31-
import java.util.Iterator;
32-
import java.util.List;
33-
import java.util.Map;
26+
import java.util.*;
3427
import java.util.Map.Entry;
35-
import java.util.Set;
28+
import java.util.concurrent.TimeUnit;
3629

3730
import com.gemstone.gemfire.GemFireIOException;
3831
import com.gemstone.gemfire.SystemFailure;
@@ -86,7 +79,6 @@
8679
import com.gemstone.gemfire.management.internal.cli.util.DiskStoreValidater;
8780
import com.gemstone.gemfire.management.internal.cli.util.MemberNotFoundException;
8881
import com.gemstone.gemfire.management.internal.messages.CompactRequest;
89-
9082
import org.springframework.shell.core.annotation.CliAvailabilityIndicator;
9183
import org.springframework.shell.core.annotation.CliCommand;
9284
import org.springframework.shell.core.annotation.CliOption;
@@ -641,20 +633,18 @@ public Result compactOfflineDiskStore(
641633
} finally {
642634
if (compacterProcess != null) {
643635
try {
644-
// just to check whether the process has exited
645-
// Process.exitValue() throws IllegalThreadStateException if Process
646-
// is alive
647-
compacterProcess.exitValue();
648-
} catch (IllegalThreadStateException ise) {
649-
// not yet terminated, destroy the process
650-
compacterProcess.destroy();
636+
// check whether the process has exited for a short period
637+
if (!compacterProcess.waitFor(100, TimeUnit.MILLISECONDS)) {
638+
// not yet terminated, destroy the process
639+
compacterProcess.destroy();
640+
}
641+
} catch (InterruptedException ignore) {
651642
}
652643
}
653644
}
654645
return result;
655646
}
656647

657-
658648
@CliCommand(value=CliStrings.UPGRADE_OFFLINE_DISK_STORE, help=CliStrings.UPGRADE_OFFLINE_DISK_STORE__HELP)
659649
@CliMetaData(shellOnly=true, relatedTopic={CliStrings.TOPIC_GEMFIRE_DISKSTORE})
660650
public Result upgradeOfflineDiskStore(
@@ -785,20 +775,18 @@ public Result upgradeOfflineDiskStore(
785775
} finally {
786776
if (upgraderProcess != null) {
787777
try {
788-
// just to check whether the process has exited
789-
// Process.exitValue() throws IllegalStateException if Process is alive
790-
upgraderProcess.exitValue();
791-
} catch (IllegalThreadStateException itse) {
792-
// not yet terminated, destroy the process
793-
upgraderProcess.destroy();
778+
// check whether the process has exited for a short period
779+
if (!upgraderProcess.waitFor(100, TimeUnit.MILLISECONDS)) {
780+
// not yet terminated, destroy the process
781+
upgraderProcess.destroy();
782+
}
783+
} catch (InterruptedException ignore) {
794784
}
795785
}
796786
}
797787
return result;
798788
}
799-
800-
801-
789+
802790
private String validatedDirectories(String[] diskDirs) {
803791
String invalidDirectories = null;
804792
StringBuilder builder = null;

0 commit comments

Comments
 (0)