mobile: Replace std::thread with Envoy::Thread::PosixThread #32610

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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

Signed-off-by: Fredy Wijaya <[email protected]>

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, alyssawilk and abeyad 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 <[email protected]>

Use std::function instead of absl::AnyInvocable

317dced

Signed-off-by: Fredy Wijaya <[email protected]>

Move the thread mocks into test/mocks/thread

49b551f

Signed-off-by: Fredy Wijaya <[email protected]>

Adjust coverage

10f0018

Signed-off-by: Fredy Wijaya <[email protected]>

Add comments

b447288

Signed-off-by: Fredy Wijaya <[email protected]>

fredyw added 3 commits March 2, 2024 16:03

Remove unncessary changes

1ff9354

Signed-off-by: Fredy Wijaya <[email protected]>

Update comment

bd17ae2

Signed-off-by: Fredy Wijaya <[email protected]>

Remove unused dep

284c29f

Signed-off-by: Fredy Wijaya <[email protected]>

abeyad reviewed Mar 4, 2024

View reviewed changes

Explicit lambda captures

0cdf0c8

Signed-off-by: Fredy Wijaya <[email protected]>

abeyad previously approved these changes Mar 4, 2024

View reviewed changes

Merge branch 'main' into posix_thread

46fbc34

Signed-off-by: Fredy Wijaya <[email protected]>

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 <[email protected]>

Fix cert path

d8ef94b

Signed-off-by: Fredy Wijaya <[email protected]>

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 <[email protected]>

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 <[email protected]>

update per_file_coverage.sh

7b500ad

Signed-off-by: Fredy Wijaya <[email protected]>

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
54 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mobile: Replace std::thread with Envoy::Thread::PosixThread #32610

mobile: Replace std::thread with Envoy::Thread::PosixThread #32610

fredyw commented Feb 27, 2024 •

edited

Loading

repokitteh-read-only bot commented Feb 27, 2024

fredyw commented Feb 28, 2024

fredyw commented Feb 28, 2024

abeyad left a comment

abeyad commented Feb 28, 2024

danzh2010 commented Feb 28, 2024

fredyw commented Mar 2, 2024

abeyad left a comment

fredyw commented Mar 5, 2024

alyssawilk left a comment

alyssawilk left a comment

abeyad left a comment

repokitteh-read-only bot commented Mar 5, 2024

abeyad commented Mar 5, 2024

abeyad commented Mar 6, 2024

mobile: Replace std::thread with Envoy::Thread::PosixThread #32610

mobile: Replace std::thread with Envoy::Thread::PosixThread #32610

Conversation

fredyw commented Feb 27, 2024 • edited Loading

repokitteh-read-only bot commented Feb 27, 2024

fredyw commented Feb 28, 2024

fredyw commented Feb 28, 2024

abeyad left a comment

Choose a reason for hiding this comment

abeyad commented Feb 28, 2024

danzh2010 commented Feb 28, 2024

fredyw commented Mar 2, 2024

abeyad left a comment

Choose a reason for hiding this comment

fredyw commented Mar 5, 2024

alyssawilk left a comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

abeyad left a comment

Choose a reason for hiding this comment

repokitteh-read-only bot commented Mar 5, 2024

abeyad commented Mar 5, 2024

abeyad commented Mar 6, 2024

fredyw commented Feb 27, 2024 •

edited

Loading