Skip to content

Commit

Permalink
Fix #1920 (port #1918)
Browse files Browse the repository at this point in the history
  • Loading branch information
@gkorland
gkorland committed Dec 25, 2018
1 parent 044717b commit 02f2cc5
Showing 1 changed file with 4 additions and 2 deletions.
6 changes: 4 additions & 2 deletions src/main/java/redis/clients/jedis/Jedis.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3629,10 +3629,12 @@ public Map<String, String> pubsubNumSub(String... channels) {
@Override
public void close() {
if (dataSource != null) {
Pool<Jedis> pool = this.dataSource;

This comment has been minimized.

Sign in to view

Copy link
@kutzi

kutzi Dec 28, 2018

Contributor

As long as access to this.dataSource is not protected by some memory barrier (synchronized, volatile), this won't help for race conditions.
I.e. the JVM is allowed to reorder the instructions.
Making this.dataSource volatile would IMO help, though.

This comment has been minimized.

Sign in to view

Copy link
@gkorland

gkorland Jan 17, 2019

Author Contributor

I don't think this is the issue here since "returnBrokenResource/returnResource" are lock based and will have the same effect as synchronized or volatile.

This comment has been minimized.

Sign in to view

Copy link
@kutzi

kutzi Jan 17, 2019

Contributor

You're right, but that's going via 2 levels and taking advantage of an implementation detail of GenericObjectPool

This comment has been minimized.

Sign in to view

Copy link
@gkorland

gkorland Jan 17, 2019

Author Contributor

No it's not, you should usually assume that working on a resource after pooled from a resource pool is thread safe and the pool is a way to synchronize the resource.
Anyway, as you can see bellow, I believe I found the issue (stupid c&p bug).

This comment has been minimized.

Sign in to view

Copy link
@kutzi

kutzi Jan 17, 2019

Contributor

An object pool could also be implemented using lock-free patterns, but I'm not trying to continue to argue about this bug.
If the change below fixes it, I'm happy with it.
this.dataSource = null;
if (client.isBroken()) {
this.dataSource.returnBrokenResource(this);
pool.returnBrokenResource(this);
} else {
this.dataSource.returnResource(this);
pool.returnResource(this);
}
this.dataSource = null;

This comment has been minimized.

Sign in to view

Copy link
@sazzad16

sazzad16 Dec 28, 2018

Collaborator

@gkorland This line should've been removed. See original commit

The backport commit has become different. Please keep the backports exactly same (or as much similar as possible) of the original commit. Squash-and-merge or rebase-and-merge, instead of simple merge, along with cherry-pick is very helpful in this kind of scenarios.

This comment has been minimized.

Sign in to view

Copy link
@kutzi

kutzi Jan 7, 2019 •
edited
Loading

Contributor

IMHO removing this line wouldn't change anything on the actual problem - i.e. that access to dataSource is not protected by any memory barrier.
See my earlier comment 02f2cc5#r31795161

This comment has been minimized.

Sign in to view

Copy link
@gkorland

gkorland Jan 17, 2019

Author Contributor

Ok I see the bug I accidentally left " this.dataSource = null;" in line 3639
} else {
Expand Down

0 comments on commit 02f2cc5

Please sign in to comment.