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.

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

fix: JENKINS-44568, JENKINS-44414 #68

Merged
merged 3 commits into from Aug 10, 2017

Conversation

Projects
None yet
4 participants
@mawinter69
Copy link
Contributor

commented Jul 25, 2017

fix: [JENKINS-44568], [JENKINS-44414]
When a message is received that is bigger than the internal buffer, the
internal buffer position was wrongly set to the buffer capacity.
But we also can't just delete the buffer is no newline was found. So we
append what we've read in a Stringbuilder and append until we encounter a
newline.

fix: JENKINS-44568, JENKINS-44414
When a message is received that is bigger than the internal buffer, the
internal buffer position was wrongly set to the buffer capacity.
But we also can't just delete the buffer is no newline was found. So we
append what we've read in a Stringbuilder and append until we encounter a
newline.
@rsandell

This comment has been minimized.

Copy link
Collaborator

commented Aug 4, 2017

So I now have three different approaches claiming to fix JENKINS-44568 and JENKINS-44414; #66, #68 and #69
That is great! but also confusing of which one is more credible. From what I can tell #68 has been soaking in the "real world" and claims to be working there and that is always a good sign and gets more credibility because of that.
But #69 actually has unit tests that verifies something along the lines of the problem so we don't get regressions in the future which makes me more inclined to merge that.
#66 has a healthy code review discussion that also makes it a bit safer to merge since it has had more eyes on it.

So which one should I go for?

@mawinter69

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2017

I know that my code works, it is running now since 2 weeks on a test instance and 1 week on a productive instance.
I have no preference, if #69 solves the problem as well I'm also fine with this.

@rsandell

This comment has been minimized.

Copy link
Collaborator

commented Aug 4, 2017

It seems like your approach is in favour at the moment it is also less intrusive.
Could you add some unit tests? Since I merged #67 you might need some fiddling to fill the buffer.

@rinrinne

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2017

One comment. If this approach is adopted, StringBuilder should be instantiated only within a method. Because StringBuilder is not thread safe.

fix a potential corruption of data
in case the data is bigger than the buffer and the data contains one or
several blanks that end up at the beginning of the buffer those blanks
would have been removed. So we have to trim only right before we return
the line.
Avoid possible performance bottle neck in case of a single event that
fills the buffer multiple times
@mawinter69

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2017

The method testReceiveEvents in GerritConnectionTest is testing that events are properly processed. The connection has a method to set the buffer size, which is set to 13 for the test. One event is created that is very big filling the buffer several times.

StringBuilder is only instantiated and used in the getLine() method.

@@ -445,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) {

This comment has been minimized.

Copy link
@mawinter69

mawinter69 Aug 4, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 7, 2017

Collaborator

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

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 7, 2017

Collaborator

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.

This comment has been minimized.

Copy link
@mawinter69

mawinter69 Aug 7, 2017

Author Contributor

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.

@@ -94,6 +94,7 @@
private AuthenticationUpdater authenticationUpdater = null;
private final Set<ConnectionListener> listeners = new CopyOnWriteArraySet<ConnectionListener>();
private int sshRxBufferSize = SSH_RX_BUFFER_SIZE;
private StringBuilder eventBuffer = null;

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 8, 2017

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

This comment has been minimized.

Copy link
@rinrinne

rinrinne Aug 8, 2017

Contributor

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)

This comment has been minimized.

Copy link
@mawinter69

mawinter69 Aug 8, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@rinrinne

rinrinne Aug 9, 2017

Contributor

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.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 9, 2017

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

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 9, 2017

@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.

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 9, 2017

Collaborator

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.

This comment has been minimized.

Copy link
@mawinter69

mawinter69 Aug 9, 2017

Author Contributor

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?

This comment has been minimized.

Copy link
@rinrinne

rinrinne Aug 9, 2017

Contributor

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

This comment has been minimized.

Copy link
@mawinter69

mawinter69 Aug 9, 2017

Author Contributor

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()
}
logger.debug("Encountered big event.");
eventBuffer = new StringBuilder();
}
eventBuffer.append(getSubSequence(cb, 0, pos));

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 8, 2017

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

This comment has been minimized.

Copy link
@mawinter69

mawinter69 Aug 8, 2017

Author Contributor

But this is only called when the buffer is full

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 9, 2017

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

@@ -336,18 +337,39 @@ private void nullifyWatchdog() {
private String getLine(CharBuffer cb) {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 8, 2017

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

@@ -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 (readCount == 0 || linecount > 0) {
sleep(SSH_RX_SLEEP_MILLIS);

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 8, 2017

It should fix JENKINS-44414 from what I see

@rsandell rsandell merged commit a0a18a3 into sonyxperiadev:master Aug 10, 2017

@rsandell

This comment has been minimized.

Copy link
Collaborator

commented Aug 10, 2017

Remembering now that I don't have Ci setup for this repo, need to fix that since there was one checkstyle warning. This time an easy fix ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.