mobile: Replace std::thread with Envoy::Thread::PosixThread by fredyw · Pull Request #32610 · envoyproxy/envoy

fredyw · 2024-02-27T22:21:35Z

This PR replaces the use of std::thread with Envoy::Thread::PosixThread (backed by pthread). The reason for this change is because std::thread may throw an exception and when exceptions are disabled, it will crash the program. For some certain code, such as the cert validation, it should not crash the program when a thread cannot be created, but it should return an error instead.

This PR also refactors the POSIX thread wrapper implementation and exposes the APIs via Envoy::Thread::PosixThreadFactory and Envoy::Thread::PosixThread so that they can be used directly by Envoy Mobile since Envoy Mobile only supports Android and iOS and those OSes support POSIX. The Envoy::Thread::PosixThread has additional functions not available in Envoy::Thread::Thread, such as pthreadId() and joinable() to make the migration from std::thread to Envoy::Thread::PosixThread easier. The Envoy::PosixThread::pthreadId() is especially useful for doing a comparison with Envoy::PosixThreadFactory::currentPthreadId().

Risk Level: medium
Testing: unit tests
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: mobile

repokitteh-read-only · 2024-02-27T22:21:40Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #32610 was opened by fredyw.

see: more, trace.

fredyw · 2024-02-28T02:51:11Z

/retest

fredyw · 2024-02-28T03:26:04Z

/assign @abeyad @alyssawilk

abeyad

this is awesome, thanks @fredyw !

mobile/library/common/thread/posix_thread.h

...test/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator_test.cc

mobile/test/common/thread/posix_thread_test.cc

abeyad · 2024-02-28T15:29:46Z

adding @danzh2010 since she wrote the original code (I think), in case she has any concerns

...le/library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.h

danzh2010 · 2024-02-28T16:50:51Z

Similarly, we can't use Envoy::Thread::Thread because, it doesn't have a way to check for the successful or failure status of the thread creation.

The Envoy::Thread::PosixThread wrapper has similar APIs with std::thread, such as PosixThread::threadId() and PosixThread::joinable() to make the migration from std::thread to Envoy::Thread::PosixThread easier.

Can you elaborate it? Envoy::Thread::Thread is just an interface and I assume PosixThread is an implementation of it.

mobile/library/common/thread/posix_thread.cc

Signed-off-by: Fredy Wijaya <fredyw@google.com>

fredyw · 2024-03-02T16:49:34Z

This PR is ready for another review. PTAL.

abeyad

awesome, thanks @fredyw !

mobile/library/common/engine_common.h

...e/library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.cc

...le/library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.h

Signed-off-by: Fredy Wijaya <fredyw@google.com>

fredyw · 2024-03-05T16:46:58Z

/retest

alyssawilk

Two thoughts for now - mostly trusting Ali on E-M changes
/wait

source/common/common/posix/thread_impl.h

source/common/common/posix/thread_impl.cc

Signed-off-by: Fredy Wijaya <fredyw@google.com>

...le/library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.h

Signed-off-by: Fredy Wijaya <fredyw@google.com>

alyssawilk

Envoy changes LGTM.

abeyad

/lgtm mobile

repokitteh-read-only · 2024-03-05T21:25:33Z

no relevant owners for "mobile"

🐱

Caused by: a #32610 (review) was submitted by @abeyad.

see: more, trace.

abeyad · 2024-03-05T21:27:02Z

added @lizan for cross company review

abeyad · 2024-03-06T15:48:39Z

added @lizan for cross company review

Actually, per the new policy, this doesn't need cross company review: https://github.com/envoyproxy/envoy/pull/30995/files

So I'm merging

* origin: (34 commits) update CODEOWNER (envoyproxy#32457) Delete unused runtime flag. (envoyproxy#32739) mobile: Use direct ByteBuffer to pass data between C++ and Java (envoyproxy#32715) quic: support cert selection by SNI, non-PEM formats (envoyproxy#32260) mobile: Replace std::thread with Envoy::Thread::PosixThread (envoyproxy#32610) grpc: Add support for max frame length in gPRC frame decoding (envoyproxy#32511) build(deps): bump google.golang.org/protobuf from 1.32.0 to 1.33.0 (envoyproxy#32728) build(deps): bump the examples-golang-network group in /examples/golang-network/simple with 1 update (envoyproxy#32732) build(deps): bump google.golang.org/protobuf from 1.32.0 to 1.33.0 in /contrib/golang/filters/http/test/test_data/property (envoyproxy#32731) build(deps): bump otel/opentelemetry-collector from `246dfe9` to `71ac13c` in /examples/opentelemetry (envoyproxy#32730) build(deps): bump the examples-grpc-bridge group in /examples/grpc-bridge/server with 2 updates (envoyproxy#32720) build(deps): bump the contrib-golang group in /contrib/golang/router/cluster_specifier/test/test_data/simple with 1 update (envoyproxy#32721) build(deps): bump the contrib-golang group in /contrib/golang/filters/http/test/test_data/echo with 1 update (envoyproxy#32722) build(deps): bump the examples-ext-authz group in /examples/ext_authz/auth/grpc-service with 1 update (envoyproxy#32723) build(deps): bump the contrib-golang group in /contrib/golang/filters/http/test/test_data/routeconfig with 1 update (envoyproxy#32724) build(deps): bump the examples-load-reporting group in /examples/load-reporting-service with 1 update (envoyproxy#32726) build(deps): bump the contrib-golang group in /contrib/golang/filters/http/test/test_data/buffer with 1 update (envoyproxy#32727) build(deps): bump the examples-golang-http group in /examples/golang-http/simple with 1 update (envoyproxy#32729) opentelemetrytracer: Add User-Agent header to OTLP trace exporters (envoyproxy#32659) build: remove incorrect cc_library after tls code move (envoyproxy#32714) ...

fredyw force-pushed the posix_thread branch 9 times, most recently from 2c7068d to 4e56de8 Compare February 28, 2024 02:19

fredyw marked this pull request as ready for review February 28, 2024 03:25

fredyw requested review from RyanTheOptimist, abeyad and alyssawilk as code owners February 28, 2024 03:25

repokitteh-read-only bot assigned abeyad and alyssawilk Feb 28, 2024

abeyad reviewed Feb 28, 2024

View reviewed changes

mobile/library/common/thread/posix_thread.h Outdated Show resolved Hide resolved

...test/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator_test.cc Outdated Show resolved Hide resolved

mobile/test/common/thread/posix_thread_test.cc Outdated Show resolved Hide resolved

abeyad assigned danzh2010 Feb 28, 2024

danzh2010 reviewed Feb 28, 2024

View reviewed changes

...le/library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.h Outdated Show resolved Hide resolved

danzh2010 reviewed Feb 28, 2024

View reviewed changes

mobile/library/common/thread/posix_thread.cc Outdated Show resolved Hide resolved

alyssawilk added the waiting:any label Feb 29, 2024

fredyw added 5 commits March 1, 2024 01:56

Replace std::thread with Thread::PosixThread

07a2e62

Signed-off-by: Fredy Wijaya <fredyw@google.com>

Use std::function instead of absl::AnyInvocable

317dced

Signed-off-by: Fredy Wijaya <fredyw@google.com>

Move the thread mocks into test/mocks/thread

49b551f

Signed-off-by: Fredy Wijaya <fredyw@google.com>

Adjust coverage

10f0018

Signed-off-by: Fredy Wijaya <fredyw@google.com>

Add comments

b447288

Signed-off-by: Fredy Wijaya <fredyw@google.com>

fredyw added 3 commits March 2, 2024 16:03

Remove unncessary changes

1ff9354

Signed-off-by: Fredy Wijaya <fredyw@google.com>

Update comment

bd17ae2

Signed-off-by: Fredy Wijaya <fredyw@google.com>

Remove unused dep

284c29f

Signed-off-by: Fredy Wijaya <fredyw@google.com>

abeyad reviewed Mar 4, 2024

View reviewed changes

Explicit lambda captures

0cdf0c8

Signed-off-by: Fredy Wijaya <fredyw@google.com>

abeyad previously approved these changes Mar 4, 2024

View reviewed changes

Merge branch 'main' into posix_thread

46fbc34

Signed-off-by: Fredy Wijaya <fredyw@google.com>

fredyw dismissed abeyad’s stale review via 46fbc34 March 5, 2024 03:12

fredyw added 2 commits March 5, 2024 15:32

Merge branch 'main' into posix_thread

1273b94

Signed-off-by: Fredy Wijaya <fredyw@google.com>

Fix cert path

d8ef94b

Signed-off-by: Fredy Wijaya <fredyw@google.com>

alyssawilk reviewed Mar 5, 2024

View reviewed changes

source/common/common/posix/thread_impl.h Show resolved Hide resolved

source/common/common/posix/thread_impl.cc Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Mar 5, 2024

Return an early nullptr

e379b63

Signed-off-by: Fredy Wijaya <fredyw@google.com>

repokitteh-read-only bot removed the waiting label Mar 5, 2024

danzh2010 reviewed Mar 5, 2024

View reviewed changes

...le/library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.h Show resolved Hide resolved

fredyw added 2 commits March 5, 2024 18:15

Add IS_ENVOY_BUG for pthread_create failure

f370bf0

Signed-off-by: Fredy Wijaya <fredyw@google.com>

update per_file_coverage.sh

7b500ad

Signed-off-by: Fredy Wijaya <fredyw@google.com>

alyssawilk approved these changes Mar 5, 2024

View reviewed changes

abeyad approved these changes Mar 5, 2024

View reviewed changes

abeyad assigned lizan Mar 5, 2024

abeyad unassigned lizan Mar 6, 2024

abeyad merged commit 4af5231 into envoyproxy:main Mar 6, 2024

fredyw deleted the posix_thread branch March 6, 2024 15:51

Conversation

fredyw commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Feb 27, 2024

Uh oh!

fredyw commented Feb 28, 2024

Uh oh!

fredyw commented Feb 28, 2024

Uh oh!

abeyad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abeyad commented Feb 28, 2024

Uh oh!

Uh oh!

danzh2010 commented Feb 28, 2024

Uh oh!

Uh oh!

fredyw commented Mar 2, 2024

Uh oh!

abeyad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fredyw commented Mar 5, 2024

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

abeyad left a comment

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Mar 5, 2024

Uh oh!

abeyad commented Mar 5, 2024

Uh oh!

abeyad commented Mar 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fredyw commented Feb 27, 2024 •

edited

Loading