Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improved code smell #63

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

improved code smell #63

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/com/rethinkdb/model/Backtrace.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ private Backtrace(List<Object> raw) {
}

public static @Nullable Backtrace fromList(List<Object> raw) {
if (raw == null || raw.size() == 0) {
if (raw == null || raw.isEmpty()) {
return null;
}
return new Backtrace(raw);
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/rethinkdb/model/Profile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ private Profile(List<Object> raw) {
}

public static @Nullable Profile fromList(List<Object> raw) {
if (raw == null || raw.size() == 0) {
if (raw == null || raw.isEmpty()) {
return null;
}
return new Profile(raw);
Expand Down
25 changes: 15 additions & 10 deletions src/main/java/com/rethinkdb/net/DefaultConnectionFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,18 +75,23 @@ SocketWrapper connect() {

// should we secure the connection?
if (sslContext != null) {
SSLSocket sslSocket = (SSLSocket) sslContext.getSocketFactory().createSocket(
socket,
socket.getInetAddress().getHostAddress(),
socket.getPort(),
true);
try (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the existing code just creates an SSLSocket and then lets it get GC'd without closing it, assigning inputStream and outputStream instead. I guess the underlying socket does get closed later.

Right now I think we don't want this try block which closes the sslSocket at the end of the block. Why would we close() the sslSocket after constructing streams from it? Wouldn't those streams stop working?

I think we'd want sslSocket to be assigned to a member variable such that its lifetime is held alive as long as the streams exist.

I'll need to investigate what's going on a little bit further before merging this PR. So right now I'm leaving this open.

SSLSocket sslSocket = (SSLSocket) sslContext.getSocketFactory()
.createSocket(
socket,
socket.getInetAddress().getHostAddress(),
socket.getPort(),
true
)
) {

// replace input/output streams
inputStream = new DataInputStream(sslSocket.getInputStream());
outputStream = sslSocket.getOutputStream();
// replace input/output streams
inputStream = new DataInputStream(sslSocket.getInputStream());
outputStream = sslSocket.getOutputStream();

// execute SSL handshake
sslSocket.startHandshake();
// execute SSL handshake
sslSocket.startHandshake();
}
} else {
outputStream = socket.getOutputStream();
inputStream = socket.getInputStream();
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/rethinkdb/net/Response.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public Response(long token, ResponseType responseType) {
}

public ReqlError makeError(Query query) {
String msg = data.size() > 0 ?
String msg = !data.isEmpty() ?
(String) data.get(0)
: "Unknown error message";
return new ErrorBuilder(msg, type)
Expand Down