Mailing-List: contact dev-help@hc.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "HttpComponents Project" <dev@hc.apache.org>
Received-SPF: neutral (athena.apache.org: 209.85.215.51 is neither permitted
 nor denied by domain of jburke@cs.washington.edu)
MIME-Version: 1.0
Date: Thu, 30 Aug 2012 12:02:22 -0700
Message-ID: 
 <CAH8S0iLUNEKP108MKA0hvxU4H+CE6VGVXAzNiUJ7JELCDpOB3A@mail.gmail.com>
Subject: @ThreadSafe BasicClientConnectionManager is not threadsafe with
 respect to shutdown
From: Jonathan Burke <jburke@cs.washington.edu>
To: dev@hc.apache.org
Content-Type: multipart/mixed; boundary=f46d04083a23c03bfd04c880504f

--f46d04083a23c03bfd04c880504f
Content-Type: multipart/alternative; boundary=f46d04083a23c03bf704c880504d

--f46d04083a23c03bf704c880504d
Content-Type: text/plain; charset=ISO-8859-1

Hello,

Perhaps it's low priority since the comments of
BasicClientConnectionManager.java does say it ought to be handled by only
one thread but I would like to point out that it is not thread safe with
respect to shutdown in case you're interested.

The attached file includes a test I added to
httpclient/src/test/java/org/apache/http/impl/conn/TestBasicConnManager.java
that will demonstrate a null pointer exception raised by potentially
improper synchronization in BasicClientConnectionManager in relatively
short order.

Problem:
Since the assignment to variable shutdown in the shutdown method (line 262)
is not part of the synchronize block and none of the assertNotShutdown
method calls are within the synchronized blocks of their enclosing methods,
it is possible to have threads execute the commands of
BasicClientConnectionManager methods in the following sequence.

Thread 1
releaseConnection (or some other method that uses assertNotShutdown is
called)
  assertNotShutdown - Passes - Line 183

Thread 2
shutdown
  the shutdown flag becomes true - Line 262
  shutdown releases this.poolEntry and this.conn in the synchronized(this)
block

Thread 1
  release connection's synchronized(this) block get's executed
  this.poolEntry is null in the try block causing a null pointer exception
- Line 211
  this.poolEntry is null in the finally block causing a null pointer
exception - Line 224

Attached is a test case that will generate the attached stack-trace when
added to TestBasicConnManager and a possible fix (though the fix has a cost
of increased synchronization).

Thank you for your time.

Jonathan

--f46d04083a23c03bf704c880504d
Content-Type: text/html; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable

<div>Hello,</div><div><br></div><div>Perhaps it&#39;s low priority since th=
e comments of BasicClientConnectionManager.java does say it ought to be han=
dled by only one thread but I would like to point out that it is not thread=
 safe with respect to shutdown in case you&#39;re interested.</div>
<div><br></div><div>The attached file includes a test I added to httpclient=
/src/test/java/org/apache/http/impl/conn/TestBasicConnManager.java that wil=
l demonstrate a null pointer exception raised by potentially improper synch=
ronization in BasicClientConnectionManager in relatively short order.</div>
<div><br></div><div>Problem:</div><div>Since the assignment to variable shu=
tdown in the shutdown method (line 262) is not part of the synchronize bloc=
k and none of the assertNotShutdown method calls are within the synchronize=
d blocks of their enclosing methods, it is possible to have threads execute=
 the commands of BasicClientConnectionManager methods in the following sequ=
ence.</div>
<div><br></div><div>Thread 1</div><div>releaseConnection (or some other met=
hod that uses assertNotShutdown is called)</div><div>=A0 assertNotShutdown =
- Passes - Line 183</div><div><br></div><div>Thread 2</div><div>shutdown</d=
iv>
<div>=A0 the shutdown flag becomes true - Line 262</div><div>=A0 shutdown r=
eleases this.poolEntry and this.conn in the synchronized(this) block</div><=
div>=A0=A0</div><div>Thread 1</div><div>=A0 release connection&#39;s synchr=
onized(this) block get&#39;s executed=A0</div>
<div>=A0 <span class=3D"Apple-tab-span" style=3D"white-space:pre">	</span>t=
his.poolEntry is null in the try block causing a null pointer exception - L=
ine 211</div><div>=A0 <span class=3D"Apple-tab-span" style=3D"white-space:p=
re">	</span>this.poolEntry is null in the finally block causing a null poin=
ter exception - Line 224</div>
<div>=A0=A0</div><div>Attached is a test case that will generate the attach=
ed stack-trace when added to TestBasicConnManager and a possible fix (thoug=
h the fix has a cost of increased synchronization).</div><div><br></div><di=
v>
Thank you for your time.</div><div><br></div><div>Jonathan</div>

--f46d04083a23c03bf704c880504d--

--f46d04083a23c03bfd04c880504f
Content-Type: text/plain; charset=US-ASCII; name="stackTrace.txt"
Content-Disposition: attachment; filename="stackTrace.txt"
Content-Transfer-Encoding: base64
X-Attachment-Id: f_h6i7zfl21

LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLQpUZXN0IHNldDogb3JnLmFwYWNoZS5odHRwLmltcGwuY29u
bi5UZXN0QmFzaWNDb25uTWFuYWdlcgotLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tClRlc3RzIHJ1bjog
NSwgRmFpbHVyZXM6IDAsIEVycm9yczogMSwgU2tpcHBlZDogMCwgVGltZSBlbGFwc2VkOiAxLjMy
NCBzZWMgPDw8IEZBSUxVUkUhCnRlc3RQcmVtYXR1cmVTaHV0ZG93bihvcmcuYXBhY2hlLmh0dHAu
aW1wbC5jb25uLlRlc3RCYXNpY0Nvbm5NYW5hZ2VyKSAgVGltZSBlbGFwc2VkOiAxLjAxNCBzZWMg
IDw8PCBFUlJPUiEKamF2YS5sYW5nLlJ1bnRpbWVFeGNlcHRpb246IEl0ZXJhdGlvbjogMTYKCWF0
IG9yZy5hcGFjaGUuaHR0cC5pbXBsLmNvbm4uVGVzdEJhc2ljQ29ubk1hbmFnZXIudGVzdFByZW1h
dHVyZVNodXRkb3duKFRlc3RCYXNpY0Nvbm5NYW5hZ2VyLmphdmE6MjY4KQoJYXQgc3VuLnJlZmxl
Y3QuTmF0aXZlTWV0aG9kQWNjZXNzb3JJbXBsLmludm9rZTAoTmF0aXZlIE1ldGhvZCkKCWF0IHN1
bi5yZWZsZWN0Lk5hdGl2ZU1ldGhvZEFjY2Vzc29ySW1wbC5pbnZva2UoTmF0aXZlTWV0aG9kQWNj
ZXNzb3JJbXBsLmphdmE6NTcpCglhdCBzdW4ucmVmbGVjdC5EZWxlZ2F0aW5nTWV0aG9kQWNjZXNz
b3JJbXBsLmludm9rZShEZWxlZ2F0aW5nTWV0aG9kQWNjZXNzb3JJbXBsLmphdmE6NDMpCglhdCBq
YXZhLmxhbmcucmVmbGVjdC5NZXRob2QuaW52b2tlKE1ldGhvZC5qYXZhOjYwMSkKCWF0IG9yZy5q
dW5pdC5ydW5uZXJzLm1vZGVsLkZyYW1ld29ya01ldGhvZCQxLnJ1blJlZmxlY3RpdmVDYWxsKEZy
YW1ld29ya01ldGhvZC5qYXZhOjQ0KQoJYXQgb3JnLmp1bml0LmludGVybmFsLnJ1bm5lcnMubW9k
ZWwuUmVmbGVjdGl2ZUNhbGxhYmxlLnJ1bihSZWZsZWN0aXZlQ2FsbGFibGUuamF2YToxNSkKCWF0
IG9yZy5qdW5pdC5ydW5uZXJzLm1vZGVsLkZyYW1ld29ya01ldGhvZC5pbnZva2VFeHBsb3NpdmVs
eShGcmFtZXdvcmtNZXRob2QuamF2YTo0MSkKCWF0IG9yZy5qdW5pdC5pbnRlcm5hbC5ydW5uZXJz
LnN0YXRlbWVudHMuSW52b2tlTWV0aG9kLmV2YWx1YXRlKEludm9rZU1ldGhvZC5qYXZhOjIwKQoJ
YXQgb3JnLmp1bml0LmludGVybmFsLnJ1bm5lcnMuc3RhdGVtZW50cy5SdW5CZWZvcmVzLmV2YWx1
YXRlKFJ1bkJlZm9yZXMuamF2YToyOCkKCWF0IG9yZy5qdW5pdC5pbnRlcm5hbC5ydW5uZXJzLnN0
YXRlbWVudHMuUnVuQWZ0ZXJzLmV2YWx1YXRlKFJ1bkFmdGVycy5qYXZhOjMxKQoJYXQgb3JnLmp1
bml0LnJ1bm5lcnMuUGFyZW50UnVubmVyLnJ1bkxlYWYoUGFyZW50UnVubmVyLmphdmE6MjYzKQoJ
YXQgb3JnLmp1bml0LnJ1bm5lcnMuQmxvY2tKVW5pdDRDbGFzc1J1bm5lci5ydW5DaGlsZChCbG9j
a0pVbml0NENsYXNzUnVubmVyLmphdmE6NjkpCglhdCBvcmcuanVuaXQucnVubmVycy5CbG9ja0pV
bml0NENsYXNzUnVubmVyLnJ1bkNoaWxkKEJsb2NrSlVuaXQ0Q2xhc3NSdW5uZXIuamF2YTo0OCkK
CWF0IG9yZy5qdW5pdC5ydW5uZXJzLlBhcmVudFJ1bm5lciQzLnJ1bihQYXJlbnRSdW5uZXIuamF2
YToyMzEpCglhdCBvcmcuanVuaXQucnVubmVycy5QYXJlbnRSdW5uZXIkMS5zY2hlZHVsZShQYXJl
bnRSdW5uZXIuamF2YTo2MCkKCWF0IG9yZy5qdW5pdC5ydW5uZXJzLlBhcmVudFJ1bm5lci5ydW5D
aGlsZHJlbihQYXJlbnRSdW5uZXIuamF2YToyMjkpCglhdCBvcmcuanVuaXQucnVubmVycy5QYXJl
bnRSdW5uZXIuYWNjZXNzJDAwMChQYXJlbnRSdW5uZXIuamF2YTo1MCkKCWF0IG9yZy5qdW5pdC5y
dW5uZXJzLlBhcmVudFJ1bm5lciQyLmV2YWx1YXRlKFBhcmVudFJ1bm5lci5qYXZhOjIyMikKCWF0
IG9yZy5qdW5pdC5ydW5uZXJzLlBhcmVudFJ1bm5lci5ydW4oUGFyZW50UnVubmVyLmphdmE6Mjky
KQoJYXQgb3JnLmFwYWNoZS5tYXZlbi5zdXJlZmlyZS5qdW5pdDQuSlVuaXQ0VGVzdFNldC5leGVj
dXRlKEpVbml0NFRlc3RTZXQuamF2YTo1MykKCWF0IG9yZy5hcGFjaGUubWF2ZW4uc3VyZWZpcmUu
anVuaXQ0LkpVbml0NFByb3ZpZGVyLmV4ZWN1dGVUZXN0U2V0KEpVbml0NFByb3ZpZGVyLmphdmE6
MTIzKQoJYXQgb3JnLmFwYWNoZS5tYXZlbi5zdXJlZmlyZS5qdW5pdDQuSlVuaXQ0UHJvdmlkZXIu
aW52b2tlKEpVbml0NFByb3ZpZGVyLmphdmE6MTA0KQoJYXQgc3VuLnJlZmxlY3QuTmF0aXZlTWV0
aG9kQWNjZXNzb3JJbXBsLmludm9rZTAoTmF0aXZlIE1ldGhvZCkKCWF0IHN1bi5yZWZsZWN0Lk5h
dGl2ZU1ldGhvZEFjY2Vzc29ySW1wbC5pbnZva2UoTmF0aXZlTWV0aG9kQWNjZXNzb3JJbXBsLmph
dmE6NTcpCglhdCBzdW4ucmVmbGVjdC5EZWxlZ2F0aW5nTWV0aG9kQWNjZXNzb3JJbXBsLmludm9r
ZShEZWxlZ2F0aW5nTWV0aG9kQWNjZXNzb3JJbXBsLmphdmE6NDMpCglhdCBqYXZhLmxhbmcucmVm
bGVjdC5NZXRob2QuaW52b2tlKE1ldGhvZC5qYXZhOjYwMSkKCWF0IG9yZy5hcGFjaGUubWF2ZW4u
c3VyZWZpcmUudXRpbC5SZWZsZWN0aW9uVXRpbHMuaW52b2tlTWV0aG9kV2l0aEFycmF5KFJlZmxl
Y3Rpb25VdGlscy5qYXZhOjE2NCkKCWF0IG9yZy5hcGFjaGUubWF2ZW4uc3VyZWZpcmUuYm9vdGVy
LlByb3ZpZGVyRmFjdG9yeSRQcm92aWRlclByb3h5Lmludm9rZShQcm92aWRlckZhY3RvcnkuamF2
YToxMTApCglhdCBvcmcuYXBhY2hlLm1hdmVuLnN1cmVmaXJlLmJvb3Rlci5TdXJlZmlyZVN0YXJ0
ZXIuaW52b2tlUHJvdmlkZXIoU3VyZWZpcmVTdGFydGVyLmphdmE6MTcyKQoJYXQgb3JnLmFwYWNo
ZS5tYXZlbi5zdXJlZmlyZS5ib290ZXIuU3VyZWZpcmVTdGFydGVyLnJ1blN1aXRlc0luUHJvY2Vz
c1doZW5Gb3JrZWQoU3VyZWZpcmVTdGFydGVyLmphdmE6MTA0KQoJYXQgb3JnLmFwYWNoZS5tYXZl
bi5zdXJlZmlyZS5ib290ZXIuRm9ya2VkQm9vdGVyLm1haW4oRm9ya2VkQm9vdGVyLmphdmE6NzAp
CkNhdXNlZCBieTogamF2YS5sYW5nLk51bGxQb2ludGVyRXhjZXB0aW9uCglhdCBvcmcuYXBhY2hl
Lmh0dHAuaW1wbC5jb25uLkJhc2ljQ2xpZW50Q29ubmVjdGlvbk1hbmFnZXIucmVsZWFzZUNvbm5l
Y3Rpb24oQmFzaWNDbGllbnRDb25uZWN0aW9uTWFuYWdlci5qYXZhOjIyNCkKCWF0IG9yZy5hcGFj
aGUuaHR0cC5pbXBsLmNvbm4uVGVzdEJhc2ljQ29ubk1hbmFnZXIudGVzdFByZW1hdHVyZVNodXRk
b3duKFRlc3RCYXNpY0Nvbm5NYW5hZ2VyLmphdmE6MjY1KQoJLi4uIDMxIG1vcmUK
--f46d04083a23c03bfd04c880504f
Content-Type: text/plain; charset=US-ASCII; name="test.txt"
Content-Disposition: attachment; filename="test.txt"
Content-Transfer-Encoding: base64
X-Attachment-Id: f_h6i7zjq92

QFRlc3QKcHVibGljIHZvaWQgdGVzdFByZW1hdHVyZVNodXRkb3duKCkgdGhyb3dzIEludGVycnVw
dGVkRXhjZXB0aW9uIHsKCiAgICAvL1RoZSBlcnJvciBkb2Vzbid0IGFsd2F5cyBoYXBwZW4gcmln
aHQgYXdheSBzaW5jZSB0aGUgb3JkZXIgb2Ygb3BlcmF0aW9ucyBjYW4gdmFyeSBiYXNlZCBvbiB0
aHJlYWQgc2NoZWR1bGluZwogICAgLy9idXQgaXQgaGFwcGVucyBmb3IgbWUgYnkgaXRlcmF0aW9u
IDI1IGluIG1vc3QgY2FzZXMKICAgIGZvcihpbnQgaSA9IDA7IGkgPCAxMDAwMDA7IGkrKykgewog
ICAgICAgIGZpbmFsIEJhc2ljQ2xpZW50Q29ubmVjdGlvbk1hbmFnZXIgbWdyID0gY3JlYXRlQ29u
bk1hbmFnZXIobnVsbCk7CgogICAgICAgIGZpbmFsIEh0dHBIb3N0IHRhcmdldCA9IGdldFNlcnZl
ckh0dHAoKTsKICAgICAgICBmaW5hbCBIdHRwUm91dGUgcm91dGUgPSBuZXcgSHR0cFJvdXRlKHRh
cmdldCwgbnVsbCwgZmFsc2UpOwoKICAgICAgICBmaW5hbCBNYW5hZ2VkQ2xpZW50Q29ubmVjdGlv
biBjb25uID0gIG1nci5nZXRDb25uZWN0aW9uKHJvdXRlLCBudWxsKTsKICAgICAgICBtZ3IucmVs
ZWFzZUNvbm5lY3Rpb24oY29ubiwgMTAwLCBUaW1lVW5pdC5NSUxMSVNFQ09ORFMpOwoKICAgICAg
ICBmaW5hbCBjbGFzcyBTaHV0ZG93blJ1bm5hYmxlIGltcGxlbWVudHMgUnVubmFibGUgewogICAg
ICAgICAgICBwdWJsaWMgdm9pZCBydW4oKSAgewogICAgICAgICAgICAgICAgdHJ5IHsKICAgICAg
ICAgICAgICAgICAgICBtZ3Iuc2h1dGRvd24oKTsKICAgICAgICAgICAgICAgIH0gY2F0Y2ggKGZp
bmFsIEV4Y2VwdGlvbiBlKSB7CiAgICAgICAgICAgICAgICAgICAgdGhyb3cgbmV3IFJ1bnRpbWVF
eGNlcHRpb24oZSk7CiAgICAgICAgICAgICAgICB9CiAgICAgICAgICAgIH0KICAgICAgICB9Cgog
ICAgICAgIC8vVGhlIHByb2JsZW0gaXMgbW9yZSByZXByb2R1Y2libGUgaWYgd2Ugc2xvdyBkb3du
IHRoZSB0aHJlYWRzIGJlbG93IHRoYXQgc3luY2hyb25pemUgb24gbWdyCiAgICAgICAgLy9UaGUg
cHJvYmxlbSBzaG91bGQgb2NjdXIgd2hlbiBzaHV0ZG93biBhbmQgcmVsZWFzZUNvbm5lY3Rpb24g
KG9yIGFueSBvdGhlciBtZXRob2QgZ3VhcmRlZCBieSBhc3NlcnROb3RTaHV0ZG93bikKICAgICAg
ICAvL2lzIGNhbGxlZAogICAgICAgIGZpbmFsIGNsYXNzIExvY2tUaGVNYW5hZ2VyIGltcGxlbWVu
dHMgUnVubmFibGUgewogICAgICAgICAgICBwdWJsaWMgdm9pZCBydW4oKSB7CiAgICAgICAgICAg
ICAgICB0cnkgewogICAgICAgICAgICAgICAgICAgIHN5bmNocm9uaXplZChtZ3IpIHsKICAgICAg
ICAgICAgICAgICAgICAgICAgVGhyZWFkLnNsZWVwKDEwMDApOyAgLy9qdXN0IHRvIHNsb3cgb3Vy
IHN5bmNocm9uaXplZCBtZXRob2RzCiAgICAgICAgICAgICAgICAgICAgfQogICAgICAgICAgICAg
ICAgfSBjYXRjaChFeGNlcHRpb24gZXhjKSB7CgogICAgICAgICAgICAgICAgfQogICAgICAgICAg
ICB9CiAgICAgICAgfQoKICAgICAgICB0cnkgewogICAgICAgICAgICBNYW5hZ2VkQ2xpZW50Q29u
bmVjdGlvbiBjb25uUmVxID0gbWdyLmdldENvbm5lY3Rpb24ocm91dGUsIG51bGwpOwogICAgICAg
ICAgICBmaW5hbCBUaHJlYWQgc2h1dGRvd24gPSBuZXcgVGhyZWFkKG5ldyBTaHV0ZG93blJ1bm5h
YmxlKCkpOwogICAgICAgICAgICBmaW5hbCBUaHJlYWQgbG9ja291dCAgPSBuZXcgVGhyZWFkKG5l
dyBMb2NrVGhlTWFuYWdlcigpKTsKCiAgICAgICAgICAgIGxvY2tvdXQuc3RhcnQoKTsKICAgICAg
ICAgICAgc2h1dGRvd24uc3RhcnQoKTsKCiAgICAgICAgICAgIG1nci5yZWxlYXNlQ29ubmVjdGlv
bihjb25uUmVxLCAxMDAwLCBUaW1lVW5pdC5NSUxMSVNFQ09ORFMpOwogICAgICAgIH0gY2F0Y2go
RXhjZXB0aW9uIGlleGMpIHsKICAgICAgICAgICAgaWYoaWV4Yy5nZXRNZXNzYWdlKCkgPT0gbnVs
bCB8fCAhaWV4Yy5nZXRNZXNzYWdlKCkuZXF1YWxzKCJDb25uZWN0aW9uIG1hbmFnZXIgaGFzIGJl
ZW4gc2h1dCBkb3duIikpIHsKICAgICAgICAgICAgICAgIHRocm93IG5ldyBSdW50aW1lRXhjZXB0
aW9uKCJGYWlsZWQgb24gaXRlcmF0aW9uOiAiICsgaSwgaWV4Yyk7CiAgICAgICAgICAgIH0KICAg
ICAgICB9CiAgICB9Cn0=

--f46d04083a23c03bfd04c880504f
Content-Type: text/plain; charset=us-ascii


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
--f46d04083a23c03bfd04c880504f--