From commits-return-22449-archive-asf-public=cust-asf.ponee.io@pulsar.apache.org Thu Feb 14 03:19:43 2019 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id 67469180763 for ; Thu, 14 Feb 2019 04:19:43 +0100 (CET) Received: (qmail 26077 invoked by uid 500); 14 Feb 2019 03:19:41 -0000 Mailing-List: contact commits-help@pulsar.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@pulsar.apache.org Delivered-To: mailing list commits@pulsar.apache.org Received: (qmail 26058 invoked by uid 99); 14 Feb 2019 03:19:41 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 14 Feb 2019 03:19:41 +0000 From: GitBox To: commits@pulsar.apache.org Subject: [GitHub] lovelle commented on a change in pull request #3581: Fix thread safety violation on ProducerImpl Message-ID: <155011438125.16137.3163710642422072960.gitbox@gitbox.apache.org> Date: Thu, 14 Feb 2019 03:19:41 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit lovelle commented on a change in pull request #3581: Fix thread safety violation on ProducerImpl URL: https://github.com/apache/pulsar/pull/3581#discussion_r256678144 ########## File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java ########## @@ -803,12 +806,12 @@ protected boolean verifyLocalBufferIsNotCorrupted(OpSendMsg op) { } protected static final class OpSendMsg { - MessageImpl msg; - List> msgs; - ByteBufPair cmd; - SendCallback callback; - long sequenceId; - long createdAt; + volatile MessageImpl msg; Review comment: > Is it too heavy to convert all fields to `volatile`? In a general sense no, it should be as cheap as load memory from L1 cache on avg, however if another thread on another core is writing the volatile field the cache-line will be invalidated and would require to load memory access from different cache access, of course this behavior depends heavily on cpu arch. Since this is done on client side and is safer than a raw variable, the overhead of this is I think is negligible. > Have you seen any issues regarding these fields? Nope, anyway, I will investigate this further and will try to add a test to reproduce this, but since it depends on very unlucky timing it might be difficult to reproduce. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: users@infra.apache.org With regards, Apache Git Services