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

[JENKINS-25218] - Hardening of FifoBuffer operation logic #100

Merged
merged 7 commits into from Nov 18, 2016

Conversation

Projects
None yet
4 participants
@oleg-nenashev
Member

oleg-nenashev commented Aug 18, 2016

The most of JENKINS-25218 has been handled in 3ea38c8 by @andresrc. But there are still potential issues in the FifoBuffer behavior.

Changes:

  • - Add closedRequested and handle it in multiple threads, which used to prevent close() calls previously
  • Fix potential NPE in FifoBuffer receiver
  • Do not rely on external code when we receive ClosedChannelException from the underlying channel

@reviewbybees

if (closed)
throw new IOException("closed during write operation");
if (closeRequested) {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Aug 18, 2016

Member

I'm still not comfortable about not having timeout if the buffer is full && does not get freed by other threads

@oleg-nenashev

oleg-nenashev Aug 18, 2016

Member

I'm still not comfortable about not having timeout if the buffer is full && does not get freed by other threads

@reviewbybees

This comment has been minimized.

Show comment
Hide comment
@reviewbybees

reviewbybees Aug 18, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

reviewbybees commented Aug 18, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Aug 23, 2016

Member

🐝 without much comprehension. I will assume this has gotten some interactive testing at least?

Member

jglick commented Aug 23, 2016

🐝 without much comprehension. I will assume this has gotten some interactive testing at least?

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Aug 23, 2016

Member

From the description, it sounds like there ought to be a way to write a test demonstrating a fix of the known bug condition.

Member

jglick commented Aug 23, 2016

From the description, it sounds like there ought to be a way to write a test demonstrating a fix of the known bug condition.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Aug 23, 2016

Member

It's being soaked for ~4 days now. Seems to be stable on frequent data interactions.

Regarding tests, I can likely reproduce b6e71fa, but it's pretty obvious. Other changes are related to some parallel code fun, so I do not see a way to reproduce them in tests without PowerMock bytecode magic.

Since it's just hardening, maybe we do not want to have it in the stable branch (excepting NPE fix)

Member

oleg-nenashev commented Aug 23, 2016

It's being soaked for ~4 days now. Seems to be stable on frequent data interactions.

Regarding tests, I can likely reproduce b6e71fa, but it's pretty obvious. Other changes are related to some parallel code fun, so I do not see a way to reproduce them in tests without PowerMock bytecode magic.

Since it's just hardening, maybe we do not want to have it in the stable branch (excepting NPE fix)

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Sep 26, 2016

Member

I decided to merge this PR towards the 3.0 branch, which will have some stabilization interval in weekly releases

Member

oleg-nenashev commented Sep 26, 2016

I decided to merge this PR towards the 3.0 branch, which will have some stabilization interval in weekly releases

@stephenc

This comment has been minimized.

Show comment
Hide comment
@stephenc

stephenc Nov 7, 2016

Member

🐝 AIUI

Member

stephenc commented Nov 7, 2016

🐝 AIUI

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Nov 9, 2016

Member

I will merge it into 3.2 since we need a stable hotfix for JENKINS-39596 in 3.1

Member

oleg-nenashev commented Nov 9, 2016

I will merge it into 3.2 since we need a stable hotfix for JENKINS-39596 in 3.1

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Nov 18, 2016

Member

The PR has been soak tested enough. 2.62.4 is unlikely going into Jenkins LTS, so I will just merge it

Member

oleg-nenashev commented Nov 18, 2016

The PR has been soak tested enough. 2.62.4 is unlikely going into Jenkins LTS, so I will just merge it

@oleg-nenashev oleg-nenashev merged commit b54adb1 into jenkinsci:stable-2.x Nov 18, 2016

1 check passed

Jenkins This pull request looks good
Details

oleg-nenashev added a commit that referenced this pull request Nov 18, 2016

oleg-nenashev added a commit that referenced this pull request Dec 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment