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

Jump to bottom

fix: JENKINS-44568, JENKINS-44414 #68

Merged
merged 3 commits into from
Aug 10, 2017
Merged

fix: JENKINS-44568, JENKINS-44414 #68

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3771a44
fix: JENKINS-44568, JENKINS-44414
mawinter69 Jul 25, 2017
ff129ad
fix a potential corruption of data
mawinter69 Aug 4, 2017
ef2078c
avoid high CPU load when no data read
mawinter69 Aug 8, 2017
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
36 changes: 31 additions & 5 deletions src/main/java/com/sonymobile/tools/gerrit/gerritevents/GerritConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,12 @@
import java.util.Set;
import java.util.concurrent.CopyOnWriteArraySet;

import com.jcraft.jsch.ChannelExec;
import com.jcraft.jsch.JSchException;
import org.codehaus.mojo.animal_sniffer.IgnoreJRERequirement;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import com.jcraft.jsch.ChannelExec;
import com.jcraft.jsch.JSchException;
import com.sonymobile.tools.gerrit.gerritevents.dto.attr.Provider;
import com.sonymobile.tools.gerrit.gerritevents.ssh.Authentication;
import com.sonymobile.tools.gerrit.gerritevents.ssh.AuthenticationUpdater;
Expand Down Expand Up @@ -94,6 +94,7 @@ public class GerritConnection extends Thread implements Connector {
private AuthenticationUpdater authenticationUpdater = null;
private final Set<ConnectionListener> listeners = new CopyOnWriteArraySet<ConnectionListener>();
private int sshRxBufferSize = SSH_RX_BUFFER_SIZE;
private StringBuilder eventBuffer = null;
Copy link

Choose a reason for hiding this comment

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

I would suggest to initialize it directly in the constructor so simplify the method logic.

StringBuilder is not thread safe, but it's perfectly fine for this case

Copy link
Contributor

Choose a reason for hiding this comment

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

StringBuilder is not thread safe, but it's perfectly fine for this case

Really???

GerritConnection con = new GerritConnection(...)
for (int i = 0; i < 3; i++) {
  Thread thread = new Thread(con)
  thread.run()
}

Thread implements Runnable. So the instance of GerritConnection can be used by multiple threads. (Though nobody want to use GerritConnection instance with such way)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually when you have configured your Jenkins against 2 Gerrit instances you have 2 GerritConnection threads running.
But for the StringBuilder this is not a problem. It is an instance variable and the only place where it is created, modified or deleted is in the private getLine method. So no problem with multiple threads.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is an instance variable and the only place where it is created, modified or deleted is in the private getLine method.

Agree if such cycle is completed within one method call. But created instance is stored into a GerritConnection instance field then return a method call. Unfortunately it is not guaranteed that getLine method call is done only by the same thread during keeping this StringBuilder instance. My sample code might reproduce such situation.

Copy link

Choose a reason for hiding this comment

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

Unfortunately it is not guaranteed that getLine method call is done only by the same thread during keeping this StringBuilder instance

If yes, it would be better to guarantee it somehow or synchronize the method. You could try using the thread-safe StringBuffer, but in the current use-case it may become a performance killer. Synchronized getLine() would be much better in this case

Copy link

Choose a reason for hiding this comment

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

@mawinter69
Well, it's not @rinrinne says:

Unfortunately it is not guaranteed that getLine method call is done only by the same thread during keeping this StringBuilder instance

I understand that it may be used by non-GerritConnection threads.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see this class intended to be used the way @rinrinne suggests, even though it could potentially be used in that way. And I don't think it is used in that way either. So documenting that it is only intended to be used by itself should suffice in that case if we think that the alternative will suffer big performance penalties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but getLine is a private method. How should a non GerritConnection thread access it? With reflection you can achieve this but why should one do it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please run this code then check displayed object hash. It is generated within private method then stored into instance field.
https://gist.github.com/rinrinne/7e40638fba102d52b1d34a7b70b16b2d

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, true but as @rsandell pointed out, using the GerritConnection in this way would not only cause problems with the StringBuilder but also with the sshConnection, the Watchdog and so on.
The normal way would be

for (int i = 0; i < 3; i++) {
  GerritConnection con = new GerritConnection(...)
  Thread thread = new Thread(con)
  thread.run()
}


/**
* Creates a GerritHandler with all the default values set.
Expand Down Expand Up @@ -336,18 +337,39 @@ private void nullifyWatchdog() {
private String getLine(CharBuffer cb) {
Copy link

Choose a reason for hiding this comment

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

Maybe makes sense to check if the message fits the buffer and just call StringBuilder if it does not. Otherwise these is a kind of dark magic in the code, which seems to hammer performance with no benefit

String line = null;
int pos = cb.position();
int limit = cb.limit();
cb.flip();
for (int i = 0; i < cb.length(); i++) {
if (cb.charAt(i) == '\n') {
line = getSubSequence(cb, 0, i).toString().trim();
line = getSubSequence(cb, 0, i).toString();
cb.position(i + 1);
break;
}
}
if (line != null) {
cb.compact();
if (eventBuffer != null) {
eventBuffer.append(line);
String eventString = eventBuffer.toString();
eventBuffer = null;
line = eventString;
}
line.trim();
} else {
cb.clear().position(pos);
if (cb.length() > 0) {
if (cb.length() == cb.capacity()) {
if (eventBuffer == null) {
logger.debug("Encountered big event.");
eventBuffer = new StringBuilder();
}
eventBuffer.append(getSubSequence(cb, 0, pos));
Copy link

Choose a reason for hiding this comment

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

It is not effective to use String Builder in such way. It would be better to push the entire chunk once you hit the limit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But this is only called when the buffer is full

Copy link

@oleg-nenashev oleg-nenashev Aug 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

right, then it's not a problem. Misread the code

} else {
cb.position(pos);
cb.limit(limit);
}
} else {
cb.clear();
}
}
return line;
}
Expand Down Expand Up @@ -409,7 +431,9 @@ public void run() {
Integer readCount;
while ((readCount = reader.read(cb)) != -1) {
logger.debug("Read count from Gerrit stream: {}", String.valueOf(readCount));
int linecount = 0;
while ((line = getLine(cb)) != null) {
linecount++;
logger.debug("Data-line from Gerrit: {}", line);
if (handler != null) {
handler.post(line, provider);
Expand All @@ -424,7 +448,9 @@ public void run() {
if (!channel.isConnected() || !sshConnection.isConnected()) {
throw new IllegalStateException("SSH connection is already lost.");
}
sleep(SSH_RX_SLEEP_MILLIS);
if (linecount > 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this a good idea. The thing is if we get an event that is 10 times the size of the buffer, it would take 1 sec to read this event in. On the other hand if for some reason the reader.read is not blocking in case no data is there or it can't fill the buffer (like in the bug we try to fix) we would consume a lot of CPU time in this thread.

Copy link
Collaborator

Choose a reason for hiding this comment

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

have you tried it in your test environment to see if there is any real performance impact?

Copy link
Collaborator

Choose a reason for hiding this comment

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

could yield() be used as an alternate to not starve the CPU? It is generally not recommended to be used, but just lifting the idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably I'm a bit too paranoid about this performance bottleneck. I encountered it while running the unit tests with a buffer size of 12, when suddently the tests started failing but output was correct. It just took too long to read everything while the unit tests started validating the calls.
This will only be a bottleneck, when we have events coming in so that the buffer is filled faster than is is emptied. That would mean with a 16kb buffer that events come in with more than 160kb/s. I guess this is rarely happening.
Personal impression is that event size is usually less than 1kb on our gerrit. But we sometimes have 7 or 8 events coming in basically at the same time, so they are all read by reader.read(cb) together.
In JENKINS-44568 it is mentioned that on gerrithub events were up to 78kb.
Maybe it is sufficient to make the buffer size configurable and mention this potential bottleneck.
What I don't know is how big the buffers of the InputStream and the Socket are. If the Socket buffer is full, gerrit side will hang up.

Maybe use
if (readCount == 0 || readCount > 0 && linecount > 0)
This way we would not sleep only when we have read data but not encountered a newline.

sleep(SSH_RX_SLEEP_MILLIS);
Copy link

Choose a reason for hiding this comment

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

It should fix JENKINS-44414 from what I see

}
}
} catch (IOException ex) {
logger.error("Stream events command error. ", ex);
Expand Down
15 changes: 11 additions & 4 deletions src/test/java/com/sonymobile/tools/gerrit/gerritevents/GerritConnectionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@
package com.sonymobile.tools.gerrit.gerritevents;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.mockito.Matchers.any;
import static org.mockito.Matchers.anyBoolean;
import static org.mockito.Matchers.eq;
Expand All @@ -47,7 +47,6 @@
import java.io.Writer;
import java.util.concurrent.CountDownLatch;

import com.jcraft.jsch.ChannelExec;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Test;
Expand All @@ -57,13 +56,13 @@
import org.powermock.core.classloader.annotations.PrepareForTest;
import org.powermock.modules.junit4.PowerMockRunner;

import com.jcraft.jsch.ChannelExec;
import com.sonymobile.tools.gerrit.gerritevents.dto.attr.Provider;
import com.sonymobile.tools.gerrit.gerritevents.ssh.Authentication;
import com.sonymobile.tools.gerrit.gerritevents.ssh.SshConnection;
import com.sonymobile.tools.gerrit.gerritevents.ssh.SshConnectionFactory;

//CS IGNORE MagicNumber FOR NEXT 200 LINES. REASON: TestData

//CS IGNORE MagicNumber FOR NEXT 300 LINES. REASON: TestData

/**
* Tests for {@link GerritConnection}.
Expand All @@ -86,6 +85,7 @@ public class GerritConnectionTest {
private static CountDownLatch downLatch = new CountDownLatch(1);

private static final String FINISH_WORD = "FINISH";

/**
* Creates a SshConnection mock and starts a GerritConnection with that connection-mock.
*
Expand All @@ -112,6 +112,7 @@ public static void setUp() throws Exception {
PowerMockito.doReturn(sshConnectionMock).when(SshConnectionFactory.class, "getConnection",
isA(String.class), isA(Integer.class), isA(String.class), isA(Authentication.class), any());
connection = new GerritConnection("", "localhost", 29418, new Authentication(null, ""));
connection.setSshRxBufferSize(13);
handlerMock = mock(HandlerMock.class);
connection.setHandler(handlerMock);
connection.addListener(new ListenerMock());
Expand Down Expand Up @@ -209,6 +210,7 @@ public void testGetGerritProxy() {
@Test
public void testReceiveEvent() throws Exception {
doCallRealMethod().when(handlerMock).post(any(String.class), any(Provider.class));
String aVeryLongMessage = "This is a very long line. It stands for a commit that contains a very huge commit message. It should be long enough to fill the buffer several times.";

Writer writer = new OutputStreamWriter(pipedOutStream);
// String
Expand All @@ -234,6 +236,9 @@ public void testReceiveEvent() throws Exception {
writer.append("{\"say\":\"hello\"}\n{\"say\":\"hello again\"}\n");
writer.flush();
Thread.sleep(500);
writer.append(aVeryLongMessage + "\n");
writer.flush();
Thread.sleep(200);
// Send finish
writer.append(FINISH_WORD);
writer.append("\n");
Expand All @@ -243,6 +248,8 @@ public void testReceiveEvent() throws Exception {
verify(handlerMock, times(2)).post(eq("{\"say\":\"hello\"}"), any(Provider.class));
verify(handlerMock, times(1)).post(eq("{\"say\":\"hello again\"}"), any(Provider.class));
verify(handlerMock, times(1)).post(eq("Thank You!"), any(Provider.class));
verify(handlerMock, times(1)).post(eq(aVeryLongMessage), any(Provider.class));
verify(handlerMock, times(7)).post(any(String.class), any(Provider.class));
}

/**
Expand Down