Skip to content

Commit 71b6495

Browse files
igfooigfoo
authored
Merge pull request github#10648 from igfoo/igfoo/lockless
Kotlin: Implement lockless TRAP writing
2 parents 8086d37 + 83a3ae6 commit 71b6495

File tree

3 files changed

+182
-36
lines changed

3 files changed

+182
-36
lines changed

‎java/kotlin-extractor/src/main/java/com/semmle/extractor/java/OdasaOutput.java

Lines changed: 174 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,16 @@
44
import java.io.File;
55
import java.io.IOException;
66
import java.util.Arrays;
7+
import java.util.Collections;
8+
import java.util.Comparator;
79
import java.util.Enumeration;
810
import java.util.HashMap;
911
import java.util.LinkedHashMap;
12+
import java.util.LinkedList;
13+
import java.util.List;
1014
import java.util.Map;
1115
import java.util.Objects;
16+
import java.util.regex.Matcher;
1217
import java.util.regex.Pattern;
1318
import java.util.zip.ZipEntry;
1419
import java.util.zip.ZipFile;
@@ -29,6 +34,7 @@
2934

3035
import com.semmle.util.concurrent.LockDirectory;
3136
import com.semmle.util.concurrent.LockDirectory.LockingMode;
37+
import com.semmle.util.data.Pair;
3238
import com.semmle.util.exception.CatastrophicError;
3339
import com.semmle.util.exception.NestedError;
3440
import com.semmle.util.exception.ResourceError;
@@ -43,6 +49,9 @@
4349
import com.semmle.util.trap.pathtransformers.PathTransformer;
4450

4551
public class OdasaOutput {
52+
// By default we use lockless TRAP writing, but this can be set
53+
// if we want to use the old TRAP locking for any reason.
54+
private final boolean use_trap_locking = Env.systemEnv().getBoolean("CODEQL_EXTRACTOR_JAVA_TRAP_LOCKING", false);
4655

4756
// either these are set ...
4857
private final File trapFolder;
@@ -260,22 +269,59 @@ private void deleteTrapFileAndDependencies(IrDeclaration sym, String signature)
260269
* Any unique suffix needed to distinguish `sym` from other declarations with the same name.
261270
* For functions for example, this means its parameter signature.
262271
*/
263-
private TrapFileManager getMembersWriterForDecl(File trap, IrDeclaration sym, String signature) {
264-
TrapClassVersion currVersion = TrapClassVersion.fromSymbol(sym, log);
265-
String shortName = sym instanceof IrDeclarationWithName ? ((IrDeclarationWithName)sym).getName().asString() : "(name unknown)";
266-
if (trap.exists()) {
267-
// Only re-write an existing trap file if we encountered a newer version of the same class.
268-
TrapClassVersion trapVersion = readVersionInfo(trap);
269-
if (!currVersion.isValid()) {
270-
log.warn("Not rewriting trap file for: " + shortName + " " + trapVersion + " " + currVersion + " " + trap);
271-
} else if (currVersion.newerThan(trapVersion)) {
272-
log.trace("Rewriting trap file for: " + shortName + " " + trapVersion + " " + currVersion + " " + trap);
273-
deleteTrapFileAndDependencies(sym, signature);
272+
private TrapFileManager getMembersWriterForDecl(File trap, File trapFileBase, TrapClassVersion trapFileVersion, IrDeclaration sym, String signature) {
273+
if (use_trap_locking) {
274+
TrapClassVersion currVersion = TrapClassVersion.fromSymbol(sym, log);
275+
String shortName = sym instanceof IrDeclarationWithName ? ((IrDeclarationWithName)sym).getName().asString() : "(name unknown)";
276+
if (trap.exists()) {
277+
// Only re-write an existing trap file if we encountered a newer version of the same class.
278+
TrapClassVersion trapVersion = readVersionInfo(trap);
279+
if (!currVersion.isValid()) {
280+
log.warn("Not rewriting trap file for: " + shortName + " " + trapVersion + " " + currVersion + " " + trap);
281+
} else if (currVersion.newerThan(trapVersion)) {
282+
log.trace("Rewriting trap file for: " + shortName + " " + trapVersion + " " + currVersion + " " + trap);
283+
deleteTrapFileAndDependencies(sym, signature);
284+
} else {
285+
return null;
286+
}
274287
} else {
275-
return null;
288+
log.trace("Writing trap file for: " + shortName + " " + currVersion + " " + trap);
276289
}
277290
} else {
278-
log.trace("Writing trap file for: " + shortName + " " + currVersion + " " + trap);
291+
// If the TRAP file already exists then we
292+
// don't need to write it.
293+
if (trap.exists()) {
294+
log.warn("Not rewriting trap file for " + trap.toString() + " as it exists");
295+
return null;
296+
}
297+
// If the TRAP file was written in the past, and
298+
// then renamed to its trap-old name, then we
299+
// don't need to rewrite it only to rename it
300+
// again.
301+
File trapFileDir = trap.getParentFile();
302+
File trapOld = new File(trapFileDir, trap.getName().replace(".trap.gz", ".trap-old.gz"));
303+
if (trapOld.exists()) {
304+
log.warn("Not rewriting trap file for " + trap.toString() + " as the trap-old exists");
305+
return null;
306+
}
307+
// Otherwise, if any newer TRAP file has already
308+
// been written then we don't need to write
309+
// anything.
310+
if (trapFileBase != null && trapFileVersion != null && trapFileDir.exists()) {
311+
String trapFileBaseName = trapFileBase.getName();
312+
313+
for (File f: FileUtil.list(trapFileDir)) {
314+
String name = f.getName();
315+
Matcher m = selectClassVersionComponents.matcher(name);
316+
if (m.matches() && m.group(1).equals(trapFileBaseName)) {
317+
TrapClassVersion v = new TrapClassVersion(Integer.valueOf(m.group(2)), Integer.valueOf(m.group(3)), Long.valueOf(m.group(4)), m.group(5));
318+
if (v.newerThan(trapFileVersion)) {
319+
log.warn("Not rewriting trap file for " + trap.toString() + " as " + f.toString() + " exists");
320+
return null;
321+
}
322+
}
323+
}
324+
}
279325
}
280326
return trapWriter(trap, sym, signature);
281327
}
@@ -328,19 +374,24 @@ public void close() {
328374
}
329375

330376
writeTrapDependencies(trapDependenciesForClass);
331-
// Record major/minor version information for extracted class files.
332-
// This is subsequently used to determine whether to re-extract (a newer version of) the same class.
333-
File metadataFile = new File(trapFile.getAbsolutePath().replace(".trap.gz", ".metadata"));
334-
try {
335-
Map<String, String> versionMap = new LinkedHashMap<>();
336-
TrapClassVersion tcv = TrapClassVersion.fromSymbol(sym, log);
337-
versionMap.put(MAJOR_VERSION, String.valueOf(tcv.getMajorVersion()));
338-
versionMap.put(MINOR_VERSION, String.valueOf(tcv.getMinorVersion()));
339-
versionMap.put(LAST_MODIFIED, String.valueOf(tcv.getLastModified()));
340-
versionMap.put(EXTRACTOR_NAME, tcv.getExtractorName());
341-
FileUtil.writePropertiesCSV(metadataFile, versionMap);
342-
} catch (IOException e) {
343-
log.warn("Could not save trap metadata file: " + metadataFile.getAbsolutePath(), e);
377+
378+
// If we are using TRAP locking then we
379+
// need to write a metadata file.
380+
if (use_trap_locking) {
381+
// Record major/minor version information for extracted class files.
382+
// This is subsequently used to determine whether to re-extract (a newer version of) the same class.
383+
File metadataFile = new File(trapFile.getAbsolutePath().replace(".trap.gz", ".metadata"));
384+
try {
385+
Map<String, String> versionMap = new LinkedHashMap<>();
386+
TrapClassVersion tcv = TrapClassVersion.fromSymbol(sym, log);
387+
versionMap.put(MAJOR_VERSION, String.valueOf(tcv.getMajorVersion()));
388+
versionMap.put(MINOR_VERSION, String.valueOf(tcv.getMinorVersion()));
389+
versionMap.put(LAST_MODIFIED, String.valueOf(tcv.getLastModified()));
390+
versionMap.put(EXTRACTOR_NAME, tcv.getExtractorName());
391+
FileUtil.writePropertiesCSV(metadataFile, versionMap);
392+
} catch (IOException e) {
393+
log.warn("Could not save trap metadata file: " + metadataFile.getAbsolutePath(), e);
394+
}
344395
}
345396
}
346397
private void writeTrapDependencies(TrapDependencies trapDependencies) {
@@ -358,6 +409,8 @@ public void setHasError() {
358409
* Trap file locking.
359410
*/
360411

412+
private final Pattern selectClassVersionComponents = Pattern.compile("(.*)#(-?[0-9]+)\\.(-?[0-9]+)-(-?[0-9]+)-(.*)\\.trap\\.gz");
413+
361414
/**
362415
* <b>CAUTION</b>: to avoid the potential for deadlock between multiple concurrent extractor processes,
363416
* only one source file {@link TrapLocker} may be open at any time, and the lock must be obtained
@@ -414,6 +467,10 @@ public TrapLocker getTrapLockerForDecl(IrDeclaration sym, String signature) {
414467
public class TrapLocker implements AutoCloseable {
415468
private final IrDeclaration sym;
416469
private final File trapFile;
470+
// trapFileBase is used when doing lockless TRAP file writing.
471+
// It is trapFile without the #metadata.trap.gz suffix.
472+
private File trapFileBase = null;
473+
private TrapClassVersion trapFileVersion = null;
417474
private final String signature;
418475
private TrapLocker(IrDeclaration decl, String signature) {
419476
this.sym = decl;
@@ -422,7 +479,20 @@ private TrapLocker(IrDeclaration decl, String signature) {
422479
log.error("Null symbol passed for Kotlin TRAP locker");
423480
trapFile = null;
424481
} else {
425-
trapFile = getTrapFileForDecl(sym, signature);
482+
File normalTrapFile = getTrapFileForDecl(sym, signature);
483+
if (use_trap_locking) {
484+
trapFile = normalTrapFile;
485+
} else {
486+
// We encode the metadata into the filename, so that the
487+
// TRAP filenames for different metadatas don't overlap.
488+
trapFileVersion = TrapClassVersion.fromSymbol(sym, log);
489+
String baseName = normalTrapFile.getName().replace(".trap.gz", "");
490+
// If a class has lots of inner classes, then we get lots of files
491+
// in a single directory. This makes our directory listings later slow.
492+
// To avoid this, rather than using files named .../Foo*, we use .../Foo/Foo*.
493+
trapFileBase = new File(new File(normalTrapFile.getParentFile(), baseName), baseName);
494+
trapFile = new File(trapFileBase.getPath() + '#' + trapFileVersion.toString() + ".trap.gz");
495+
}
426496
}
427497
}
428498
private TrapLocker(File jarFile) {
@@ -437,20 +507,83 @@ private TrapLocker(String moduleName) {
437507
}
438508
public TrapFileManager getTrapFileManager() {
439509
if (trapFile!=null) {
440-
lockTrapFile(trapFile);
441-
return getMembersWriterForDecl(trapFile, sym, signature);
510+
if (use_trap_locking) {
511+
lockTrapFile(trapFile);
512+
}
513+
return getMembersWriterForDecl(trapFile, trapFileBase, trapFileVersion, sym, signature);
442514
} else {
443515
return null;
444516
}
445517
}
518+
446519
@Override
447520
public void close() {
448521
if (trapFile!=null) {
449522
try {
450-
unlockTrapFile(trapFile);
523+
if (use_trap_locking) {
524+
unlockTrapFile(trapFile);
525+
}
451526
} catch (NestedError e) {
452527
log.warn("Error unlocking trap file " + trapFile.getAbsolutePath(), e);
453528
}
529+
530+
// If we are writing TRAP file locklessly, then now that we
531+
// have finished writing our TRAP file, we want to rename
532+
// and TRAP file that matches our trapFileBase but doesn't
533+
// have the latest metadata.
534+
// Renaming it to trap-old means that it won't be imported,
535+
// but we can still use its presence to avoid future
536+
// invocations rewriting it, and it means that the information
537+
// is in the TRAP directory if we need it for debugging.
538+
if (!use_trap_locking && sym != null) {
539+
File trapFileDir = trapFileBase.getParentFile();
540+
String trapFileBaseName = trapFileBase.getName();
541+
542+
List<Pair<File, TrapClassVersion>> pairs = new LinkedList<Pair<File, TrapClassVersion>>();
543+
for (File f: FileUtil.list(trapFileDir)) {
544+
String name = f.getName();
545+
Matcher m = selectClassVersionComponents.matcher(name);
546+
if (m.matches()) {
547+
if (m.group(1).equals(trapFileBaseName)) {
548+
TrapClassVersion v = new TrapClassVersion(Integer.valueOf(m.group(2)), Integer.valueOf(m.group(3)), Long.valueOf(m.group(4)), m.group(5));
549+
pairs.add(new Pair<File, TrapClassVersion>(f, v));
550+
} else {
551+
// Everything in this directory should be for the same TRAP file base
552+
log.error("Unexpected sibling " + m.group(1) + " when extracting " + trapFileBaseName);
553+
}
554+
}
555+
}
556+
if (pairs.isEmpty()) {
557+
log.error("Wrote TRAP file, but no TRAP files exist for " + trapFile.getAbsolutePath());
558+
} else {
559+
Comparator<Pair<File, TrapClassVersion>> comparator = new Comparator<Pair<File, TrapClassVersion>>() {
560+
@Override
561+
public int compare(Pair<File, TrapClassVersion> p1, Pair<File, TrapClassVersion> p2) {
562+
TrapClassVersion v1 = p1.snd();
563+
TrapClassVersion v2 = p2.snd();
564+
if (v1.equals(v2)) {
565+
return 0;
566+
} else if (v1.newerThan(v2)) {
567+
return 1;
568+
} else {
569+
return -1;
570+
}
571+
}
572+
};
573+
TrapClassVersion latestVersion = Collections.max(pairs, comparator).snd();
574+
575+
for (Pair<File, TrapClassVersion> p: pairs) {
576+
if (!latestVersion.equals(p.snd())) {
577+
File f = p.fst();
578+
File fOld = new File(f.getParentFile(), f.getName().replace(".trap.gz", ".trap-old.gz"));
579+
// We aren't interested in whether or not this succeeds;
580+
// it may fail because a concurrent extractor has already
581+
// renamed it.
582+
f.renameTo(fOld);
583+
}
584+
}
585+
}
586+
}
454587
}
455588
}
456589

@@ -505,6 +638,17 @@ private TrapClassVersion(int majorVersion, int minorVersion, long lastModified,
505638
this.lastModified = lastModified;
506639
this.extractorName = extractorName;
507640
}
641+
642+
@Override
643+
public boolean equals(Object obj) {
644+
if (obj instanceof TrapClassVersion) {
645+
TrapClassVersion other = (TrapClassVersion)obj;
646+
return majorVersion == other.majorVersion && minorVersion == other.minorVersion && lastModified == other.lastModified && extractorName.equals(other.extractorName);
647+
} else {
648+
return false;
649+
}
650+
}
651+
508652
private boolean newerThan(TrapClassVersion tcv) {
509653
// Classes being compiled from source have major version 0 but should take precedence
510654
// over any classes with the same qualified name loaded from the classpath

‎java/ql/integration-tests/linux-only/kotlin/custom_plugin/diagnostics.expected

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,3 @@
11
| CodeQL Kotlin extractor | 2 | | IrProperty without a getter | d.kt:0:0:0:0 | d.kt:0:0:0:0 |
2-
| CodeQL Kotlin extractor | 2 | | Not rewriting trap file for: Boolean -1.0-0- -1.0-0-null test-db/trap/java/classes/kotlin/Boolean.members.trap.gz | file://:0:0:0:0 | file://:0:0:0:0 |
2+
| CodeQL Kotlin extractor | 2 | | Not rewriting trap file for test-db/trap/java/classes/java/lang/Boolean.members/Boolean.members<VERSION>-<MODIFIED>-kotlin.trap.gz as it exists | file://:0:0:0:0 | file://:0:0:0:0 |
3+
| CodeQL Kotlin extractor | 2 | | Not rewriting trap file for test-db/trap/java/classes/kotlin/Boolean.members/Boolean.members<VERSION>-<MODIFIED>-null.trap.gz as it exists | file://:0:0:0:0 | file://:0:0:0:0 |

‎java/ql/integration-tests/linux-only/kotlin/custom_plugin/diagnostics.ql

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,14 @@
11
import java
22

3-
from string genBy, int severity, string tag, string msg, Location l
3+
from string genBy, int severity, string tag, string msg, string msg2, Location l
44
where
55
diagnostics(_, genBy, severity, tag, msg, _, l) and
66
(
77
// Different installations get different sets of these messages,
88
// so we filter out all but one that happens everywhere.
9-
msg.matches("Not rewriting trap file for: %")
9+
msg.matches("Not rewriting trap file for %")
1010
implies
11-
msg.matches("Not rewriting trap file for: Boolean %")
12-
)
13-
select genBy, severity, tag, msg, l
11+
msg.matches("Not rewriting trap file for %Boolean.members%")
12+
) and
13+
msg2 = msg.regexpReplaceAll("#-?[0-9]+\\.-?[0-9]+--?[0-9]+-", "<VERSION>-<MODIFIED>-")
14+
select genBy, severity, tag, msg2, l

0 commit comments

Comments
 (0)