okhttp: Removed Annotation SuppressWarnings("GuardedBy") #11620
Conversation
vinodhabib
changed the title
| // TODO(b/145386688): This access should be guarded by 'stream.transportState().lock'; instead | ||
| // found: 'this.lock' | ||
| stream.transportState().start(streamFactory); | ||
| synchronized (lock) { |
There was a problem hiding this comment.
synchronized (lock) already exists at the caller site (newStream method) and should not be necessary to add.
There was a problem hiding this comment.
I can see a query raised with Eric as below , hope as of now no action required. please confirm?
#6578 (comment)
FYI, I can also see a note in the issue description as below.
If we can't remove them or if it would make the code worse, we can leave them as-is and close this issue.
| stream.transportState().start(streamFactory); | ||
| synchronized (lock) { | ||
| streams.add(stream); | ||
| synchronized (stream.transportState().lock) { |
There was a problem hiding this comment.
stream.transportState().lock is never accessed outside of synchronized(lock) so this change should also not be needed.
There was a problem hiding this comment.
I can see a query raised with Eric as below , hope as of now no action required. please confirm?
#6578 (comment)
There was a problem hiding this comment.
@shivaspeaks @kannanjgithub I can see Eric replied on last raised query #6578 (comment) as below.
I wasn't suggesting we remove necessary locks. I was suggesting organize the code such that the tooling can verify the locking. stream.transportState().lock is the same object as this.lock, but ErrorProne isn't able to figure that out because it requires data flow analysis.
I have made similar changes in current PR. Kindly suggest how to proceed further on the same.
Removed the SuppressWarnings("GuardedBy") Annotation and Code cleanup.
Note : I can see some UT's are failing in RetriableStreamTest to the corresponding changes of RetriableStream(due to circular dependency issues) which still i am working, meanwhile request you to have look into other 3 files which are completed.
Completed Files:
CronetClientTransport
OkHttpClientStream
OkHttpClientTransport
Pending File:
RetriableStream (Due to Circular Dependency of failing UT's which has both retriableStream.start() and retriableStream.cancel() methods)
Fixes #6578