You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Aravind-Suresh (via GitHub)" <gi...@apache.org> on 2023/07/28 17:38:39 UTC
[GitHub] [pinot] Aravind-Suresh opened a new issue, #11207: Race condition / undefined behavior in LLRealtimeSegmentDataManager
Aravind-Suresh opened a new issue, #11207:
URL: https://github.com/apache/pinot/issues/11207
In the LLRealtimeSegmentDataManager constructor, we invoke `startConsumerThread` before the constructor returns. So, "this" escapes into the thread before completing the construction and hence there could be race conditions where the new thread sees an uninitialized portion of "this".
While testing a patch locally in debugger, I ran into this race condition and looks like the member variable "_leaseExtender" is used inside `PartitionConsumer` that's started in a new thread.
Now, in case of such races, that new thread could see "_leaseExtender" as null and would result in NPE. This could result in the consumer thread getting stopped for that Kafka partition.
Relevant code is pasted below.
```
public LLRealtimeSegmentDataManager(SegmentZKMetadata segmentZKMetadata, TableConfig tableConfig,
RealtimeTableDataManager realtimeTableDataManager, String resourceDataDir, IndexLoadingConfig indexLoadingConfig,
Schema schema, LLCSegmentName llcSegmentName, Semaphore partitionGroupConsumerSemaphore,
ServerMetrics serverMetrics, @Nullable PartitionUpsertMetadataManager partitionUpsertMetadataManager,
@Nullable PartitionDedupMetadataManager partitionDedupMetadataManager, BooleanSupplier isReadyToConsumeData) {
_segBuildSemaphore = realtimeTableDataManager.getSegmentBuildSemaphore();
_segmentZKMetadata = segmentZKMetadata;
::::
// this causes the problem as we invoke it within the constructor
startConsumerThread();
}
::::
protected void startConsumerThread() {
_consumerThread = new Thread(new PartitionConsumer(), _segmentNameStr);
_segmentLogger.info("Created new consumer thread {} for {}", _consumerThread, this);
_consumerThread.start();
}
::::
// this is a non-static class, so "this" escapes if we start this runnable
// in a new thread before it gets initialized
public class PartitionConsumer implements Runnable {
public void run() {
```
The solution would be to move this call to a separate function and calling that post the constructor call in all the usages. Will raise a fix for this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on issue #11207: Race condition / undefined behavior in LLRealtimeSegmentDataManager
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on issue #11207:
URL: https://github.com/apache/pinot/issues/11207#issuecomment-1658705991
Thanks for submitting and researching into the issue. To better understand the issue, is this caused by the potential reordering of the instructions by JVM in the constructor that causes the `startConsumerThread()` to happen before `_leaseExtender` is initialized?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang closed issue #11207: Race condition / undefined behavior in LLRealtimeSegmentDataManager
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang closed issue #11207: Race condition / undefined behavior in LLRealtimeSegmentDataManager
URL: https://github.com/apache/pinot/issues/11207
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Aravind-Suresh commented on issue #11207: Race condition / undefined behavior in LLRealtimeSegmentDataManager
Posted by "Aravind-Suresh (via GitHub)" <gi...@apache.org>.
Aravind-Suresh commented on issue #11207:
URL: https://github.com/apache/pinot/issues/11207#issuecomment-1659480262
Yes it could be due to reordering of instructions / changes not flushed to main memory before the new thread starts (this flushing to main memory synchronously is done specifically for volatile variables). At least for final variables, Java guarantees this "flush" after the constructor returns.
This particular section explains with an example the type of guarantees Java provides - http://www.cs.umd.edu/~pugh/java/memoryModel/jsr-133-faq.html#finalRight
In any case it's better to not let "this" escape before the constructor returns to avoid undefined behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org