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

SeverityLevelCounter should be thread-safe #4927

Closed
soon opened this Issue Aug 10, 2017 · 5 comments

Comments

@soon
Contributor

soon commented Aug 10, 2017

Now the SeverityLevelCounter uses int count to track the number of errors with the specified SeverityLevel. However, this is not thread-safe, because int incrementation is not an atomic operation and therefore may lead to unpredictable results.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 10, 2017

Member

@soon SeverityLevelCounter is an AuditListener.
Will all listeners be multi-threaded? How will this affect text and xml output in DefaultLogger and XmlLogger?
Won't we need to synchronize these areas to prevent garbling the output?

Member

rnveach commented Aug 10, 2017

@soon SeverityLevelCounter is an AuditListener.
Will all listeners be multi-threaded? How will this affect text and xml output in DefaultLogger and XmlLogger?
Won't we need to synchronize these areas to prevent garbling the output?

@soon

This comment has been minimized.

Show comment
Hide comment
@soon

soon Aug 10, 2017

Contributor

@rnveach, Yes, I'm preparing a PR for XmlLogger. There is a problem with fileStarted and fileFinished methods.

DefaultLogger works fine, according to my tests. Seems like the PrintWriter synchronizes write operations, however, I haven't found a documentation for this behavior. I'll also prepare a PR for DefaultLogger, this seems to be an easy fix

Contributor

soon commented Aug 10, 2017

@rnveach, Yes, I'm preparing a PR for XmlLogger. There is a problem with fileStarted and fileFinished methods.

DefaultLogger works fine, according to my tests. Seems like the PrintWriter synchronizes write operations, however, I haven't found a documentation for this behavior. I'll also prepare a PR for DefaultLogger, this seems to be an easy fix

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 10, 2017

Member

Seems like the PrintWriter synchronizes write operations, however, I haven't found a documentation for this behavior.

http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/io/PrintWriter.java#427
I am seeing synchronizations all over.
Maybe as long as the full line is given, it won't bash 2 lines together. I would be careful if we only print portions of the line though.

Member

rnveach commented Aug 10, 2017

Seems like the PrintWriter synchronizes write operations, however, I haven't found a documentation for this behavior.

http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/io/PrintWriter.java#427
I am seeing synchronizations all over.
Maybe as long as the full line is given, it won't bash 2 lines together. I would be careful if we only print portions of the line though.

@soon

This comment has been minimized.

Show comment
Hide comment
@soon

soon Aug 11, 2017

Contributor

I am seeing synchronizations all over.

The documentation says only about lock field inside Writer class (https://docs.oracle.com/javase/7/docs/api/java/io/Writer.html):

The object used to synchronize operations on this stream.

However, the PrintWriter class does not give any guarantees about thread-safety (https://docs.oracle.com/javase/7/docs/api/java/io/PrintWriter.html#write(char[]))

Contributor

soon commented Aug 11, 2017

I am seeing synchronizations all over.

The documentation says only about lock field inside Writer class (https://docs.oracle.com/javase/7/docs/api/java/io/Writer.html):

The object used to synchronize operations on this stream.

However, the PrintWriter class does not give any guarantees about thread-safety (https://docs.oracle.com/javase/7/docs/api/java/io/PrintWriter.html#write(char[]))

@sabaka sabaka moved this from ToDo to In Review in Multi-thread mode for Java files processing Aug 14, 2017

@rnveach rnveach added the approved label Aug 15, 2017

romani added a commit that referenced this issue Aug 17, 2017

@romani romani added the bug label Aug 17, 2017

@romani romani added this to the 8.2 milestone Aug 17, 2017

@romani romani changed the title from `SeverityLevelCounter` should be thread-safe to SeverityLevelCounter should be thread-safe Aug 17, 2017

@sabaka sabaka moved this from In Review to Done in Multi-thread mode for Java files processing Aug 18, 2017

@sabaka

This comment has been minimized.

Show comment
Hide comment
@sabaka

sabaka Aug 18, 2017

Contributor

merged

Contributor

sabaka commented Aug 18, 2017

merged

@sabaka sabaka closed this Aug 18, 2017

ArneLimburg pushed a commit to ArneLimburg/checkstyle that referenced this issue Aug 23, 2017

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