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

Synchronize access to list created with synchronizedList when iterating #786

Open
wants to merge 1 commit into
base: master
from

Conversation

4 participants
@kinow

kinow commented Nov 22, 2017

Hello,

No ticket associated. Was just randomly looking at some code, and as recently worked on a ticket in another project for similar issue, remembered about this one.

I believe ACQUIRED_SLOTS, which was created with Collections.synchronizedList, must be accessed within a synchronized block when iterating, according to Javadocs.

Even though the javadocs for synchronizedList don't mention it, the synchronizedCollection mentions streams() as well as Iterator.

Hope that helps
Bruno

ps: Eclipse pointed that warning was not happening anymore. In case IntelliJ says otherwise, I'd be happy to put it back (though I suspect IntelliJ won't complain about it as well)

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Nov 22, 2017

Codecov Report

Merging #786 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #786      +/-   ##
============================================
- Coverage     53.77%   53.75%   -0.02%     
  Complexity     1511     1511              
============================================
  Files           296      296              
  Lines          8341     8343       +2     
  Branches        727      727              
============================================
  Hits           4485     4485              
- Misses         3610     3612       +2     
  Partials        246      246
Impacted Files Coverage Δ Complexity Δ
...alando/nakadi/service/ConsumerLimitingService.java 8% <0%> (-0.17%) 2 <0> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb30f88...db77585. Read the comment docs.

codecov-io commented Nov 22, 2017

Codecov Report

Merging #786 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #786      +/-   ##
============================================
- Coverage     53.77%   53.75%   -0.02%     
  Complexity     1511     1511              
============================================
  Files           296      296              
  Lines          8341     8343       +2     
  Branches        727      727              
============================================
  Hits           4485     4485              
- Misses         3610     3612       +2     
  Partials        246      246
Impacted Files Coverage Δ Complexity Δ
...alando/nakadi/service/ConsumerLimitingService.java 8% <0%> (-0.17%) 2 <0> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb30f88...db77585. Read the comment docs.

@lmontrieux

This comment has been minimized.

Show comment
Hide comment
@lmontrieux

lmontrieux Nov 23, 2017

Member

Thank you for your contribution, @kinow . We will review it ASAP.

Member

lmontrieux commented Nov 23, 2017

Thank you for your contribution, @kinow . We will review it ASAP.

@lmontrieux lmontrieux added the bug label Nov 23, 2017

@lmontrieux

This comment has been minimized.

Show comment
Hide comment
@lmontrieux

lmontrieux Nov 24, 2017

Member

👍

Member

lmontrieux commented Nov 24, 2017

👍

1 similar comment
@adyach

This comment has been minimized.

Show comment
Hide comment
@adyach

adyach Nov 28, 2017

Member

👍

Member

adyach commented Nov 28, 2017

👍

@lmontrieux

This comment has been minimized.

Show comment
Hide comment
@lmontrieux

lmontrieux Dec 12, 2017

Member

Deploy validation please

Member

lmontrieux commented Dec 12, 2017

Deploy validation please

@lmontrieux

This comment has been minimized.

Show comment
Hide comment
@lmontrieux

lmontrieux Dec 12, 2017

Member

We have to fix our deployment script to validate this properly - sorry about the delay

Member

lmontrieux commented Dec 12, 2017

We have to fix our deployment script to validate this properly - sorry about the delay

@kinow

This comment has been minimized.

Show comment
Hide comment
@kinow

kinow Dec 12, 2017

Let me know if there's anything I can do to help. The coverage will decrease as there are more lines, even though they would be covered by previous tests.

kinow commented Dec 12, 2017

Let me know if there's anything I can do to help. The coverage will decrease as there are more lines, even though they would be covered by previous tests.

@lmontrieux

This comment has been minimized.

Show comment
Hide comment
@lmontrieux

lmontrieux Dec 15, 2017

Member

The issue is with the deployment process we use to validate pull requests. We have a fix ready, as soon as it is applied we will be able to validate your PR. Sorry for the delay.

Member

lmontrieux commented Dec 15, 2017

The issue is with the deployment process we use to validate pull requests. We have a fix ready, as soon as it is applied we will be able to validate your PR. Sorry for the delay.

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