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

Report a possible race issue #3647

Closed
baigd opened this Issue Sep 22, 2015 · 3 comments

Comments

Projects
None yet
4 participants
@baigd

baigd commented Sep 22, 2015

Hi, Developers of facebook/presto,

I am writing to report a race issue on use of ConcurrentHashMap. The issue is reported by our tool in an automatic way. Although manually confirmed, it would be a false positive, given we do not know the specification of the program. We would very appreciate if you could check below for details and confirm with us whether it is a real problem. For more information, please refer to our website: http://sav.sutd.edu.sg/?page_id=2845

File: facebook/presto/presto-blackhole/src/main/java/com/facebook/presto/plugin/blackhole/BlackHoleMetadata.java
Location: Line (149/150,172/136)
Description:
The remove-then-put operations on "tables" in line 149 and 150 are guarded by the lock "tables". If the intention is to guarantee the atomicity of the remove-then-put operations, then the write operations in line 136, 172 may break this atomicity. Relying on the ConcurrentHashMap to ensure exclusive access is dangerous since ConcurrentHashMap has no guarantee of exclusive access.

@losipiuk

This comment has been minimized.

Show comment
Hide comment
@losipiuk

losipiuk Sep 22, 2015

Member

It seems to me that synchronization in BlackHoleMetadata class is not needed at all. Even if all accesses to tables map were done in synchronized section we could still run into race conditions on metadata access.

To avoid that we would need add synchronization mechanisms in *Task classes (e.g. CreateTableTask) or even higher in DataDefinitionExecution.

The downside of that is that ConnectorMetadata interface would probably become much less user friendly.

Member

losipiuk commented Sep 22, 2015

It seems to me that synchronization in BlackHoleMetadata class is not needed at all. Even if all accesses to tables map were done in synchronized section we could still run into race conditions on metadata access.

To avoid that we would need add synchronization mechanisms in *Task classes (e.g. CreateTableTask) or even higher in DataDefinitionExecution.

The downside of that is that ConnectorMetadata interface would probably become much less user friendly.

@kokosing

This comment has been minimized.

Show comment
Hide comment
@kokosing

kokosing Sep 25, 2015

Member

Simple approach of fixing that would be to try implement transaction-like
metadata storage with using ConnectorSession.getStartTime() as transaction id (txid)
Then session with higher txid would need to wait for all
operations with lower txid to finish.

What do you think about it?

Member

kokosing commented Sep 25, 2015

Simple approach of fixing that would be to try implement transaction-like
metadata storage with using ConnectorSession.getStartTime() as transaction id (txid)
Then session with higher txid would need to wait for all
operations with lower txid to finish.

What do you think about it?

kokosing added a commit to Teradata/presto that referenced this issue Sep 25, 2015

blackhole: remove synchronization of tables in BlackHoleMetadata
Synchronization (atomic remove and put into concurrent hash map of
tables) is not needed, it does not guarantee that concurrent operations
on metadata can be run safely (e.g. rename table with drop table).

Simple approach of fixing that would be to try implement transaction-like
metadata storage with using ConnectorSession.getStartTime() as transaction id (txid)
Then session with higher txid would need to wait for all
operations with lower txid to finish.

Ticket: prestodb#3647

Test Plan: none

Reviewers: losipiuk

@martint martint closed this in 0631fa4 Oct 11, 2015

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