New issue
Advanced search Search tips

Issue 897867 link

Starred by 1 user
Status: Fixed
Owner:
pmarko@chromium.org
Closed: Jan 14
Cc:
pbond@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

diagnostics: Fix sequence checks in grpc_async_adapter Debug/Release

Project Member Reported by emaxx@chromium.org, Oct 22 
The grpc_async_adapter library (src/platform2/diagnostics/grpc_async_adapter/) attempts to enforce the sequence checks not only in the Debug mode, but also in the Release mode:

  CHECK(sequence_checker_.CalledOnValidSequence());

(for the reason that this library is also intended to be used by third parties - so that there's a higher risk of it being misused).

However, seems that these assertions are currently no-op in the Release mode, since SequenceChecker transforms into SequenceCheckerDoNothing when the DCHECKs are disabled.

We should either transform these CHECKs into DCHECKs, or change the code to instantiate the SequenceCheckerImpl class directly.

Comment 1 by pmarko@chromium.org, Oct 23

Status: Started (was: Assigned)
Ah, thanks for catching this Maksim!! Let's just use SequenceCheckerImpl directly.
It is essential that the SequenceChecker works in the Release builds. Otherwise, if the sequencing assumptions are broken, things may misbehave and it would be hard to track down the root cause, esp. since we won't be able to see the source code.

Comment 2 by pmarko@chromium.org, Nov 26

Cc: pbond@chromium.org
Labels: Sarien

Comment 3 by bartfab@chromium.org, Nov 26

Labels: Enterprise-Triaged
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 27 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/b1778c39aca41dcc5fc597a8ee9612e1c6d746e1

commit b1778c39aca41dcc5fc597a8ee9612e1c6d746e1
Author: Pavol Marko <pmarko@chromium.org>
Date: Tue Nov 27 04:31:24 2018

diagnostics: grpc_async_adapter: Use sequence checks in release builds

Make sequence checks effective in release builds by explicitly using
SequenceCheckerImpl. This is important because third-party code will be
using the grpc_async_adapter.

BUG= chromium:897867 
TEST=none

Change-Id: I048a2d6d6aa38e852cbd52f0a8e86c219e4195e3
Reviewed-on: https://chromium-review.googlesource.com/1296629
Commit-Ready: Pavol Marko <pmarko@chromium.org>
Tested-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>

[modify] https://crrev.com/b1778c39aca41dcc5fc597a8ee9612e1c6d746e1/diagnostics/grpc_async_adapter/grpc_completion_queue_dispatcher.h

Comment 5 by pmarko@chromium.org, Jan 14

Status: Fixed (was: Started)
►

Sign in to add a comment