Skip to content

Commit 0675d6d

Browse files
adamretteradamretter
committed
[refactor] Make sure that XML:DB Remote and XML-RPC APIs do not leak query resources
1 parent 80f8d20 commit 0675d6d

9 files changed

Lines changed: 497 additions & 484 deletions

File tree

‎exist-core/pom.xml‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1237,11 +1237,15 @@
12371237
<include>src/test/java/org/exist/xmldb/concurrent/action/ValueAppendAction.java</include>
12381238
<include>src/test/java/org/exist/xmldb/concurrent/action/XQueryAction.java</include>
12391239
<include>src/test/java/org/exist/xmldb/concurrent/action/XQueryUpdateAction.java</include>
1240+
<include>src/main/java/org/exist/xmlrpc/AbstractCachedResult.java</include>
1241+
<include>src/main/java/org/exist/xmlrpc/CachedQueryResult.java</include>
12401242
<include>src/main/java/org/exist/xmlrpc/ExistRpcTypeFactory.java</include>
12411243
<include>src/test/java/org/exist/xmlrpc/MimeTypeTest.java</include>
1244+
<include>src/main/java/org/exist/xmlrpc/QueryResultCache.java</include>
12421245
<include>src/test/java/org/exist/xmlrpc/QuerySessionTest.java</include>
12431246
<include>src/main/java/org/exist/xmlrpc/RpcAPI.java</include>
12441247
<include>src/main/java/org/exist/xmlrpc/RpcConnection.java</include>
1248+
<include>src/main/java/org/exist/xmlrpc/XmldbRequestProcessorFactory.java</include>
12451249
<include>src/test/java/org/exist/xmlrpc/XmlRpcTest.java</include>
12461250
<include>src/main/java/org/exist/xqj/Marshaller.java</include>
12471251
<include>src/test/java/org/exist/xqj/MarshallerTest.java</include>
@@ -2140,15 +2144,19 @@
21402144
<exclude>src/test/java/org/exist/xmldb/concurrent/action/ValueAppendAction.java</exclude>
21412145
<exclude>src/test/java/org/exist/xmldb/concurrent/action/XQueryAction.java</exclude>
21422146
<exclude>src/test/java/org/exist/xmldb/concurrent/action/XQueryUpdateAction.java</exclude>
2147+
<exclude>src/main/java/org/exist/xmlrpc/AbstractCachedResult.java</exclude>
21432148
<exclude>src/main/java/org/exist/xmlrpc/ACEAiderParser.java</exclude>
21442149
<exclude>src/main/java/org/exist/xmlrpc/ACEAiderSerializer.java</exclude>
21452150
<exclude>src/main/java/org/exist/xmlrpc/ArrayWrapperParser.java</exclude>
21462151
<exclude>src/main/java/org/exist/xmlrpc/ArrayWrapperSerializer.java</exclude>
2152+
<exclude>src/main/java/org/exist/xmlrpc/CachedQueryResult.java</exclude>
21472153
<exclude>src/main/java/org/exist/xmlrpc/ExistRpcTypeFactory.java</exclude>
21482154
<exclude>src/test/java/org/exist/xmlrpc/MimeTypeTest.java</exclude>
2155+
<exclude>src/main/java/org/exist/xmlrpc/QueryResultCache.java</exclude>
21492156
<exclude>src/test/java/org/exist/xmlrpc/QuerySessionTest.java</exclude>
21502157
<exclude>src/main/java/org/exist/xmlrpc/RpcAPI.java</exclude>
21512158
<exclude>src/main/java/org/exist/xmlrpc/RpcConnection.java</exclude>
2159+
<exclude>src/main/java/org/exist/xmlrpc/XmldbRequestProcessorFactory.java</exclude>
21522160
<exclude>src/main/java/org/exist/xmlrpc/XmlRpcExtensionConstants.java</exclude>
21532161
<exclude>src/test/java/org/exist/xmlrpc/XmlRpcTest.java</exclude>
21542162
<exclude>src/main/java/org/exist/xqj/Marshaller.java</exclude>

‎exist-core/src/main/java/org/exist/xmlrpc/AbstractCachedResult.java‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,28 @@
11
/*
2+
* Elemental
3+
* Copyright (C) 2024, Evolved Binary Ltd
4+
*
5+
* admin@evolvedbinary.com
6+
* https://www.evolvedbinary.com | https://www.elemental.xyz
7+
*
8+
* This library is free software; you can redistribute it and/or
9+
* modify it under the terms of the GNU Lesser General Public
10+
* License as published by the Free Software Foundation; version 2.1.
11+
*
12+
* This library is distributed in the hope that it will be useful,
13+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
14+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
15+
* Lesser General Public License for more details.
16+
*
17+
* You should have received a copy of the GNU Lesser General Public
18+
* License along with this library; if not, write to the Free Software
19+
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
20+
*
21+
* NOTE: Parts of this file contain code from 'The eXist-db Authors'.
22+
* The original license header is included below.
23+
*
24+
* =====================================================================
25+
*
226
* eXist-db Open Source Native XML Database
327
* Copyright (C) 2001 The eXist-db Authors
428
*
@@ -37,7 +61,7 @@ public abstract class AbstractCachedResult implements Closeable {
3761
protected long queryTime = 0;
3862
protected long creationTimestamp = 0;
3963
protected long timestamp = 0;
40-
private boolean closed;
64+
private boolean closed = false;
4165

4266
public AbstractCachedResult() {
4367
this(0);

‎exist-core/src/main/java/org/exist/xmlrpc/CachedQueryResult.java‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
/*
2+
* Elemental
3+
* Copyright (C) 2024, Evolved Binary Ltd
4+
*
5+
* admin@evolvedbinary.com
6+
* https://www.evolvedbinary.com | https://www.elemental.xyz
7+
*
8+
* This library is free software; you can redistribute it and/or
9+
* modify it under the terms of the GNU Lesser General Public
10+
* License as published by the Free Software Foundation; version 2.1.
11+
*
12+
* This library is distributed in the hope that it will be useful,
13+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
14+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
15+
* Lesser General Public License for more details.
16+
*
17+
* You should have received a copy of the GNU Lesser General Public
18+
* License along with this library; if not, write to the Free Software
19+
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
20+
*
21+
* NOTE: Parts of this file contain code from 'The eXist-db Authors'.
22+
* The original license header is included below.
23+
*
24+
* =====================================================================
25+
*
26+
* eXist-db Open Source Native XML Database
27+
* Copyright (C) 2001 The eXist-db Authors
28+
*
29+
* info@exist-db.org
30+
* http://www.exist-db.org
31+
*
32+
* This library is free software; you can redistribute it and/or
33+
* modify it under the terms of the GNU Lesser General Public
34+
* License as published by the Free Software Foundation; either
35+
* version 2.1 of the License, or (at your option) any later version.
36+
*
37+
* This library is distributed in the hope that it will be useful,
38+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
39+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
40+
* Lesser General Public License for more details.
41+
*
42+
* You should have received a copy of the GNU Lesser General Public
43+
* License along with this library; if not, write to the Free Software
44+
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
45+
*/
46+
package org.exist.xmlrpc;
47+
48+
import org.exist.xquery.XQueryUtil;
49+
import org.exist.xquery.value.Sequence;
50+
51+
import java.util.Properties;
52+
53+
import org.apache.logging.log4j.LogManager;
54+
import org.apache.logging.log4j.Logger;
55+
56+
import javax.annotation.Nullable;
57+
58+
/**
59+
* Simple container for the results of a query. Used to cache
60+
* query results that may be retrieved later by the client.
61+
*
62+
* @author wolf
63+
* @author jmfernandez
64+
* @author <a href="mailto:adam@evolvedbinary.com">Adam Retter</a>
65+
*/
66+
public class CachedQueryResult extends AbstractCachedResult {
67+
68+
private static final Logger LOG = LogManager.getLogger(CachedQueryResult.class);
69+
private final XQueryUtil.QueryResult queryResult;
70+
private @Nullable final Properties serialization;
71+
72+
public CachedQueryResult(final XQueryUtil.QueryResult queryResult, @Nullable final Properties outputProperties) {
73+
super(queryResult.totalTime());
74+
this.queryResult = queryResult;
75+
this.serialization = outputProperties;
76+
}
77+
78+
/**
79+
* Get the total time to produce the query result.
80+
* This is compilation time (if any) plus the execution time.
81+
*
82+
* @return total time to produce the query result.
83+
*/
84+
public long totalTime() {
85+
return queryResult.totalTime();
86+
}
87+
88+
@Override
89+
public Sequence getResult() {
90+
return queryResult.result;
91+
}
92+
93+
public @Nullable Properties getSerialization() {
94+
return serialization;
95+
}
96+
97+
@Override
98+
protected void doClose() {
99+
queryResult.close();
100+
}
101+
}

‎exist-core/src/main/java/org/exist/xmlrpc/QueryResult.java‎

Lines changed: 0 additions & 96 deletions
This file was deleted.

‎exist-core/src/main/java/org/exist/xmlrpc/QueryResultCache.java‎

Lines changed: 42 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,28 @@
11
/*
2+
* Elemental
3+
* Copyright (C) 2024, Evolved Binary Ltd
4+
*
5+
* admin@evolvedbinary.com
6+
* https://www.evolvedbinary.com | https://www.elemental.xyz
7+
*
8+
* This library is free software; you can redistribute it and/or
9+
* modify it under the terms of the GNU Lesser General Public
10+
* License as published by the Free Software Foundation; version 2.1.
11+
*
12+
* This library is distributed in the hope that it will be useful,
13+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
14+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
15+
* Lesser General Public License for more details.
16+
*
17+
* You should have received a copy of the GNU Lesser General Public
18+
* License along with this library; if not, write to the Free Software
19+
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
20+
*
21+
* NOTE: Parts of this file contain code from 'The eXist-db Authors'.
22+
* The original license header is included below.
23+
*
24+
* =====================================================================
25+
*
226
* eXist-db Open Source Native XML Database
327
* Copyright (C) 2001 The eXist-db Authors
428
*
@@ -23,11 +47,12 @@
2347

2448
import com.github.benmanes.caffeine.cache.Cache;
2549
import com.github.benmanes.caffeine.cache.Caffeine;
50+
import com.github.benmanes.caffeine.cache.RemovalListener;
2651
import net.jcip.annotations.ThreadSafe;
2752
import org.apache.logging.log4j.LogManager;
2853
import org.apache.logging.log4j.Logger;
2954

30-
import java.util.Date;
55+
import javax.annotation.Nullable;
3156
import java.util.concurrent.TimeUnit;
3257
import java.util.concurrent.atomic.AtomicInteger;
3358

@@ -43,19 +68,23 @@ public class QueryResultCache {
4368
private static final Logger LOG = LogManager.getLogger(QueryResultCache.class);
4469
private static final int TIMEOUT = 180_000; // ms (e.g. 2 minutes)
4570

71+
private static final RemovalListener<Integer, AbstractCachedResult> REMOVAL_LISTENER = (cacheId, cachedResult, removalCause) -> {
72+
if (LOG.isDebugEnabled()) {
73+
LOG.debug("Removing cached query result for cache: {}", cacheId);
74+
}
75+
76+
// NOTE(AR) make sure to release any resources still held by the cached result
77+
cachedResult.close();
78+
};
79+
4680
private final AtomicInteger cacheIdCounter = new AtomicInteger();
4781
private final Cache<Integer, AbstractCachedResult> cache;
4882

4983
public QueryResultCache() {
5084
this.cache = Caffeine.newBuilder()
5185
.expireAfterAccess(TIMEOUT, TimeUnit.MILLISECONDS)
52-
.removalListener((key, value, cause) -> {
53-
final AbstractCachedResult qr = (AbstractCachedResult)value;
54-
qr.close(); // must close associated resources
55-
if(LOG.isDebugEnabled()) {
56-
LOG.debug("Removing cached result set: {}", new Date(qr.getTimestamp()));
57-
}
58-
}).build();
86+
.removalListener(REMOVAL_LISTENER)
87+
.build();
5988
}
6089

6190
public int add(final AbstractCachedResult qr) {
@@ -64,24 +93,24 @@ public int add(final AbstractCachedResult qr) {
6493
return cacheId;
6594
}
6695

67-
public AbstractCachedResult get(final int cacheId) {
96+
public @Nullable AbstractCachedResult get(final int cacheId) {
6897
if (cacheId < 0 || cacheId >= cacheIdCounter.get()) {
6998
return null;
7099
}
71100
return cache.getIfPresent(cacheId);
72101
}
73102

74-
public QueryResult getResult(final int cacheId) {
103+
public @Nullable CachedQueryResult getResult(final int cacheId) {
75104
final AbstractCachedResult acr = get(cacheId);
76-
return (acr != null && acr instanceof QueryResult) ? (QueryResult) acr : null;
105+
return (acr != null && acr instanceof CachedQueryResult) ? (CachedQueryResult) acr : null;
77106
}
78107

79-
public SerializedResult getSerializedResult(final int cacheId) {
108+
public @Nullable SerializedResult getSerializedResult(final int cacheId) {
80109
final AbstractCachedResult acr = get(cacheId);
81110
return (acr != null && acr instanceof SerializedResult) ? (SerializedResult) acr : null;
82111
}
83112

84-
public CachedContentFile getCachedContentFile(final int cacheId) {
113+
public @Nullable CachedContentFile getCachedContentFile(final int cacheId) {
85114
final AbstractCachedResult acr = get(cacheId);
86115
return (acr != null && acr instanceof CachedContentFile) ? (CachedContentFile) acr : null;
87116
}

0 commit comments

Comments
 (0)