Skip to content

Commit 72071da

Browse files
rschwietzkerbri
rschwietzke
authored and
rbri
committed
Updated CSS3Parser pool implementation to lower synchronization, prevent leaks and use the AutoCloseable interface instead of Closeable
1 parent 3a1c389 commit 72071da

4 files changed

Lines changed: 261 additions & 46 deletions

File tree

‎src/main/java/org/htmlunit/WebClient.java‎

Lines changed: 114 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@
2727
import static org.htmlunit.BrowserVersionFeatures.WINDOW_EXECUTE_EVENTS;
2828

2929
import java.io.BufferedInputStream;
30-
import java.io.Closeable;
3130
import java.io.File;
3231
import java.io.IOException;
3332
import java.io.InputStream;
@@ -55,6 +54,7 @@
5554
import java.util.Objects;
5655
import java.util.Optional;
5756
import java.util.Set;
57+
import java.util.concurrent.ConcurrentLinkedDeque;
5858
import java.util.concurrent.Executor;
5959
import java.util.concurrent.ExecutorService;
6060
import java.util.concurrent.Executors;
@@ -151,6 +151,7 @@
151151
* @author Anton Demydenko
152152
* @author Sergio Moreno
153153
* @author Lai Quang Duong
154+
* @author René Schwietzke
154155
*/
155156
public class WebClient implements Serializable, AutoCloseable {
156157

@@ -206,9 +207,8 @@ public class WebClient implements Serializable, AutoCloseable {
206207
private OnbeforeunloadHandler onbeforeunloadHandler_;
207208
private Cache cache_ = new Cache();
208209

209-
// mini pool to save resource when parsing css
210-
private transient PooledCSS3Parser firstPooledCSS3Parser_ = new PooledCSS3Parser();
211-
private transient PooledCSS3Parser secondPooledCSS3Parser_ = new PooledCSS3Parser();
210+
// mini pool to save resource when parsing CSS
211+
private transient CSS3ParserPool css3ParserPool = new CSS3ParserPool();
212212

213213
/** target "_blank". */
214214
public static final String TARGET_BLANK = "_blank";
@@ -2954,46 +2954,120 @@ public XHtmlPage loadXHtmlCodeIntoCurrentWindow(final String xhtmlCode) throws I
29542954
/**
29552955
* <span style="color:red">INTERNAL API - SUBJECT TO CHANGE AT ANY TIME - USE AT YOUR OWN RISK.</span><br>
29562956
*
2957-
* @return CSS3Parser from pool
2957+
* @return a CSS3Parser that will return to an internal pool for reuse if closed using the
2958+
* try-with-resource concept
29582959
*/
2959-
public PooledCSS3Parser getCSS3ParserFromPool() {
2960-
if (!firstPooledCSS3Parser_.inUse_) {
2961-
if (firstPooledCSS3Parser_.open()) {
2962-
return firstPooledCSS3Parser_;
2963-
}
2964-
}
2965-
if (!secondPooledCSS3Parser_.inUse_) {
2966-
if (secondPooledCSS3Parser_.open()) {
2967-
return secondPooledCSS3Parser_;
2968-
}
2969-
}
2970-
2971-
return new PooledCSS3Parser();
2960+
public PooledCSS3Parser getCSS3Parser() {
2961+
return this.css3ParserPool.get();
29722962
}
29732963

2974-
public static class PooledCSS3Parser implements Closeable {
2975-
private final CSS3Parser css3Parser_;
2976-
private boolean inUse_;
2977-
2978-
public PooledCSS3Parser() {
2979-
css3Parser_ = new CSS3Parser();
2980-
}
2981-
2982-
public CSS3Parser css3Parser() {
2983-
return css3Parser_;
2984-
}
2985-
2986-
public synchronized boolean open() {
2987-
if (inUse_) {
2988-
return false;
2989-
}
2990-
inUse_ = true;
2991-
return true;
2992-
}
2993-
2964+
/**
2965+
* Our pool of CSS3Parsers. If you need a parser, get it from here and use the AutoCloseable
2966+
* functionality with a try-with-resource block. If you don't want to do that at all, continue
2967+
* to build CSS3Parsers the old fashioned way.
2968+
*
2969+
* Fetching a parser is thread safe. This API is built to minimize synchronization overhead,
2970+
* hence it is possible to miss a returned parser from another thread under heavy pressure,
2971+
* but because that is unlikely, we keep it simple and efficient. Caches are not supposed
2972+
* to give cutting-edge guarantees.
2973+
*
2974+
* This concept avoids a resource leak when someone does not close the fetched
2975+
* parser because the pool does not know anything about the parser unless
2976+
* it returns. We are not running a checkout-checkin concept.
2977+
*
2978+
* <span style="color:red">INTERNAL API - SUBJECT TO CHANGE AT ANY TIME - USE AT YOUR OWN RISK.</span><br>
2979+
*/
2980+
public static class CSS3ParserPool {
2981+
/*
2982+
* Our pool. We only hold data when it is available. In addition, synchronization against
2983+
* this deque is cheap.
2984+
*/
2985+
private ConcurrentLinkedDeque<PooledCSS3Parser> parsers = new ConcurrentLinkedDeque<>();
2986+
2987+
/**
2988+
* Fetch a new or recycled CSS3parser. Make sure you use the try-with-resource concept
2989+
* to automatically return it after use because a parser creation is expensive.
2990+
* We won't get a leak, if you don't do so, but that will remove the advantage.
2991+
*
2992+
* @return a parser
2993+
*/
2994+
public PooledCSS3Parser get() {
2995+
// see if we have one, LIFO
2996+
final PooledCSS3Parser parser = parsers.pollLast();
2997+
2998+
// if we don't have one, get us one
2999+
return parser != null ? parser.markInUse(this) : new PooledCSS3Parser(this);
3000+
}
3001+
3002+
/**
3003+
* Return a parser. Normally you don't have to use that method explicitly.
3004+
* Prefer to user the AutoCloseable interface of the PooledParser by
3005+
* using a try-with-resource statement.
3006+
*
3007+
* @param parser the parser to recycle
3008+
*/
3009+
protected void recycle(final PooledCSS3Parser parser) {
3010+
parsers.addLast(parser);
3011+
}
3012+
}
3013+
3014+
/**
3015+
* This is a poolable CSS3Parser which can be reused automatically when closed.
3016+
* A regular CSS3Parser is not thread-safe, hence also our pooled parser
3017+
* is not thread-safe.
3018+
*
3019+
* <span style="color:red">INTERNAL API - SUBJECT TO CHANGE AT ANY TIME - USE AT YOUR OWN RISK.</span><br>
3020+
*/
3021+
public static class PooledCSS3Parser extends CSS3Parser implements AutoCloseable {
3022+
/**
3023+
* The pool we want to return us to
3024+
*/
3025+
private CSS3ParserPool pool;
3026+
3027+
/**
3028+
* Create a new poolable parser
3029+
*
3030+
* @param pool the pool the parser should return to when it is closed
3031+
*/
3032+
protected PooledCSS3Parser(final CSS3ParserPool pool) {
3033+
super();
3034+
this.pool = pool;
3035+
}
3036+
3037+
/**
3038+
* Resets the parser's pool state so it can be safely returned again
3039+
*
3040+
* @param pool the pool the parser should return to when it is closed
3041+
*/
3042+
protected PooledCSS3Parser markInUse(final CSS3ParserPool pool) {
3043+
// ensure we detect programming mistakes, Java will optimize this
3044+
// null check away when it never happens
3045+
if (this.pool == null) {
3046+
this.pool = pool;
3047+
}
3048+
else {
3049+
throw new IllegalStateException("This PooledParser was not returned to the pool properly");
3050+
}
3051+
3052+
return this;
3053+
}
3054+
3055+
/**
3056+
* Implements the AutoClosable interface. The return method ensures that
3057+
* we are notified when we incorrectly close it twice which indicates a
3058+
* programming flow defect.
3059+
*
3060+
* @throws IllegalStateException in case the parser is closed several times
3061+
*/
29943062
@Override
2995-
public void close() throws IOException {
2996-
inUse_ = false;
3063+
public void close() {
3064+
if (this.pool != null) {
3065+
this.pool.recycle(this);
3066+
this.pool = null;
3067+
}
3068+
else {
3069+
throw new IllegalStateException("This PooledParser was returned already");
3070+
}
29973071
}
29983072
}
29993073
}

‎src/main/java/org/htmlunit/css/CssStyleSheet.java‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1012,9 +1012,11 @@ private static boolean getNthElement(final String nth, final int index) {
10121012
*/
10131013
private static CSSStyleSheetImpl parseCSS(final InputSource source, final WebClient client) {
10141014
CSSStyleSheetImpl ss;
1015-
try (PooledCSS3Parser pooledParser = client.getCSS3ParserFromPool()) {
1015+
1016+
// use a pooled parser, if any available to avoid expensive recreation
1017+
try (final PooledCSS3Parser pooledParser = client.getCSS3Parser()) {
10161018
final CSSErrorHandler errorHandler = client.getCssErrorHandler();
1017-
final CSSOMParser parser = new CSSOMParser(pooledParser.css3Parser());
1019+
final CSSOMParser parser = new CSSOMParser(pooledParser);
10181020
parser.setErrorHandler(errorHandler);
10191021
ss = parser.parseStyleSheet(source, null);
10201022
}
@@ -1041,8 +1043,9 @@ public static MediaListImpl parseMedia(final String mediaString, final WebClient
10411043
return media;
10421044
}
10431045

1044-
try (PooledCSS3Parser pooledParser = webClient.getCSS3ParserFromPool()) {
1045-
final CSSOMParser parser = new CSSOMParser(pooledParser.css3Parser());
1046+
// get us a pooled parser for efficiency because a new parser is expensive
1047+
try (final PooledCSS3Parser pooledParser = webClient.getCSS3Parser()) {
1048+
final CSSOMParser parser = new CSSOMParser(pooledParser);
10461049
parser.setErrorHandler(webClient.getCssErrorHandler());
10471050

10481051
media = new MediaListImpl(parser.parseMedia(mediaString));

‎src/main/java/org/htmlunit/html/DomNode.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1864,8 +1864,10 @@ public DomNodeList<DomNode> querySelectorAll(final String selectors) {
18641864
*/
18651865
protected SelectorList getSelectorList(final String selectors, final WebClient webClient)
18661866
throws IOException {
1867-
try (PooledCSS3Parser pooledParser = webClient.getCSS3ParserFromPool()) {
1868-
final CSSOMParser parser = new CSSOMParser(pooledParser.css3Parser());
1867+
1868+
// get us a CSS3Parser from the pool so the chance of reusing it are high
1869+
try (final PooledCSS3Parser pooledParser = webClient.getCSS3Parser()) {
1870+
final CSSOMParser parser = new CSSOMParser(pooledParser);
18691871
final CheckErrorHandler errorHandler = new CheckErrorHandler();
18701872
parser.setErrorHandler(errorHandler);
18711873

‎src/test/java/org/htmlunit/WebClient9Test.java‎

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
/*
2+
* Copyright (c) 2002-2023 Gargoyle Software Inc.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
* https://www.apache.org/licenses/LICENSE-2.0
8+
*
9+
* Unless required by applicable law or agreed to in writing, software
10+
* distributed under the License is distributed on an "AS IS" BASIS,
11+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
* See the License for the specific language governing permissions and
13+
* limitations under the License.
14+
*/
15+
package org.htmlunit;
16+
17+
import org.htmlunit.WebClient.PooledCSS3Parser;
18+
import org.htmlunit.cssparser.parser.javacc.CSS3Parser;
19+
import org.htmlunit.junit.BrowserRunner;
20+
import org.junit.Test;
21+
import org.junit.runner.RunWith;
22+
23+
/**
24+
* Tests for {@link WebClient} and its CSS3Parser pool
25+
*
26+
* @author René Schwietzke
27+
*/
28+
@RunWith(BrowserRunner.class)
29+
public class WebClient9Test extends SimpleWebTestCase {
30+
31+
/**
32+
* Ensure that we always get fresh parser when there
33+
* is none and we already fetched one.
34+
*/
35+
@Test
36+
public void newParsersWhenNotReturning() {
37+
try (WebClient webClient = new WebClient()) {
38+
// ask for a parser
39+
CSS3Parser p1 = webClient.getCSS3Parser();
40+
41+
// if we ask again, it is not the same
42+
CSS3Parser p2 = webClient.getCSS3Parser();
43+
44+
assertFalse(p1 == p2);
45+
}
46+
}
47+
48+
/**
49+
* When we close a parser, we get it again when
50+
* asking the next time
51+
*/
52+
@SuppressWarnings("resource")
53+
@Test
54+
public void closedParserIsPooled() {
55+
try (WebClient webClient = new WebClient()) {
56+
// ask for a parser
57+
PooledCSS3Parser p1;
58+
PooledCSS3Parser p2;
59+
60+
try (PooledCSS3Parser p = webClient.getCSS3Parser()) {
61+
assertNotNull(p);
62+
p1 = p;
63+
}
64+
try (PooledCSS3Parser p = webClient.getCSS3Parser()) {
65+
assertNotNull(p);
66+
p2 = p;
67+
assertTrue(p == p1);
68+
}
69+
try (PooledCSS3Parser p = webClient.getCSS3Parser()) {
70+
assertNotNull(p);
71+
assertTrue(p == p2);
72+
assertTrue(p1 == p2);
73+
}
74+
}
75+
}
76+
77+
/**
78+
* We can nest and get properly different parsers
79+
*/
80+
@SuppressWarnings("resource")
81+
@Test
82+
public void nestingWorks() {
83+
try (WebClient webClient = new WebClient()) {
84+
PooledCSS3Parser p1;
85+
PooledCSS3Parser p2;
86+
87+
try (PooledCSS3Parser p11 = webClient.getCSS3Parser()) {
88+
try (PooledCSS3Parser p21 = webClient.getCSS3Parser()) {
89+
assertNotNull(p11);
90+
assertNotNull(p21);
91+
assertFalse(p11 == p21);
92+
93+
// keep them
94+
p1 = p11;
95+
p2 = p21;
96+
}
97+
}
98+
99+
try (PooledCSS3Parser p11 = webClient.getCSS3Parser()) {
100+
try (PooledCSS3Parser p21 = webClient.getCSS3Parser()) {
101+
assertNotNull(p11);
102+
assertNotNull(p21);
103+
assertFalse(p11 == p21);
104+
105+
assertTrue(p11 == p1);
106+
assertTrue(p21 == p2);
107+
}
108+
}
109+
}
110+
}
111+
112+
/**
113+
* Take one, returned it, need two... get another new one
114+
*/
115+
@SuppressWarnings("resource")
116+
@Test
117+
public void flow1_2() {
118+
try (WebClient webClient = new WebClient()) {
119+
PooledCSS3Parser p1;
120+
121+
try (PooledCSS3Parser p11 = webClient.getCSS3Parser()) {
122+
p1 = p11;
123+
}
124+
125+
try (PooledCSS3Parser p11 = webClient.getCSS3Parser()) {
126+
assertNotNull(p11);
127+
assertTrue(p11 == p1);
128+
129+
try (PooledCSS3Parser p21 = webClient.getCSS3Parser()) {
130+
assertNotNull(p21);
131+
assertFalse(p21 == p11);
132+
}
133+
}
134+
}
135+
}
136+
}

0 commit comments

Comments
 (0)