Add native-platform callback implementation #316

goaway · 2019-08-01T22:25:35Z

Signed-off-by: Mike Schore mike.schore@gmail.com

library/common/include/c_types.h

library/swift/src/Client.swift

library/objective-c/EnvoyTypes.h

library/objective-c/EnvoyEngine.mm

library/objective-c/EnvoyEngine.h

rebello95

Great work! Let's ship and iterate on master.

library/common/include/c_types.h

library/objective-c/EnvoyHttpStream.h

library/objective-c/BUILD

rebello95 · 2019-08-03T05:48:20Z

library/objective-c/EnvoyHttpStream.m

+    if (atomic_load(c->canceled)) {
+      return;
+    }
+  });


Shouldn't this be calling back up to the platform?

Yep, some of this is more of a sketch; working headers is the main goal for this PR (with the framework in place to get the rest working).

rebello95 · 2019-08-03T05:50:10Z

library/objective-c/EnvoyTypes.h

@@ -91,6 +98,9 @@ typedef struct {
 */
 @property (nonatomic, strong) void (^onError)(EnvoyEngineError *error);

+// FIXME
+@property (nonatomic, strong) void (^onCancel)();
+


Should we add one for onComplete too? Or is the assumption that onTrailers encompasses this?

Current plan is not to surface onComplete, since it's actually meant more to trigger internal bookkeeping. Other callbacks have the boolean on them and trailers/error imply closure already.

That said we can surface this if it ends up being necessary/useful. Currently though, it will be a little bit redundant.

library/swift/src/Client.swift

library/objective-c/BUILD

library/objective-c/EnvoyEngineImpl.h

rebello95 · 2019-08-05T21:57:36Z

library/objective-c/EnvoyEngineImpl.h

+
+/// Performs necessary setup after Envoy has initialized and started running.
+/// TODO: create a post-initialization callback from Envoy to handle this automatically.
+ (void)setupEnvoy;


If this is defined here, should probably also be in the EnvoyEngine interface

I kept this here because the plan is to get rid of it as soon as we have the post-init callback wired up.

Sounds good

rebello95 · 2019-08-05T22:05:07Z

library/swift/src/Envoy.swift

@@ -1,7 +1,7 @@
 import Foundation

 @objcMembers
-public final class Envoy: NSObject {
+public final class Envoy<Engine: EnvoyEngine>: NSObject {


I don't think this needs to be generic. @buildbreaker and I were considering just instantiating this class with an EnvoyEngine.Type (for class methods, or an instance of EnvoyEngine)

It doesn't need to be I guess. Are there downsides?

I went this direction because it felt a little cleaner to me than passing the type as an initialization parameter that needs to be stored and referenced, esp. when the interface is static.

Maybe it makes more sense in the context that our current thinking is that EnvoyEngine will always be static, and Envoy will be the platform instance.

Generics are harder to pass around in platform code, because everything that uses them also needs to be generic. For example, we'd have to do:

func foo<T: EnvoyEngine>(_ envoy: Envoy<T>)

Instead of:

func foo(_ envoy: Envoy)

Reducing the generic and instead keeping the type hidden within the object will remove the need for this

But nothing else should actually need the generic type right? Like foo doesn't need to know the specialized type of Envoy. Pretty sure func foo(_ envoy: Envoy) should still work(?)

library/objective-c/BUILD

rebello95

Great work getting this set up. Let's merge and iterate.

Signed-off-by: Mike Schore <mike.schore@gmail.com>

Signed-off-by: Jose Nino <jnino@lyft.com>

Signed-off-by: Mike Schore <mike.schore@gmail.com>

bazel/swift_static_framework.bzl

Signed-off-by: Mike Schore <mike.schore@gmail.com>

rebello95 · 2019-08-09T19:35:16Z

looks like there's a lint failure on CI but green otherwise!

Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway · 2019-08-09T19:38:18Z

pushed lint fix.

swift library tests all pass locally for me, so I think this is effectively green.

rebello95 · 2019-08-09T19:38:47Z

I'm fine with going ahead and merging

This was originally going to be done as part of #316, but we ran into build issues. Making this change will allow us to mock `EnvoyEngine` from the Swift/Kotlin platform layers. Signed-off-by: Michael Rebello <me@michaelrebello.com>

This was originally going to be done as part of envoyproxy/envoy-mobile#316, but we ran into build issues. Making this change will allow us to mock `EnvoyEngine` from the Swift/Kotlin platform layers. Signed-off-by: Michael Rebello <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.com>

goaway changed the title ~~wip~~ [wip] Add native-platform callback implementation Aug 1, 2019

rebello95 reviewed Aug 1, 2019

View reviewed changes

library/objective-c/EnvoyEngine.mm Outdated Show resolved Hide resolved

rebello95 reviewed Aug 2, 2019

View reviewed changes

library/objective-c/EnvoyEngine.h Outdated Show resolved Hide resolved

rebello95 previously approved these changes Aug 3, 2019

View reviewed changes

goaway dismissed rebello95’s stale review via 234bb9e August 5, 2019 21:48

rebello95 reviewed Aug 5, 2019

View reviewed changes

library/objective-c/BUILD Outdated Show resolved Hide resolved

rebello95 reviewed Aug 5, 2019

View reviewed changes

library/objective-c/EnvoyEngineImpl.h Outdated Show resolved Hide resolved

rebello95 reviewed Aug 5, 2019

View reviewed changes

goaway marked this pull request as ready for review August 5, 2019 22:05

goaway changed the title ~~[wip] Add native-platform callback implementation~~ Add native-platform callback implementation Aug 5, 2019

rebello95 previously approved these changes Aug 5, 2019

View reviewed changes

goaway dismissed rebello95’s stale review via e778006 August 5, 2019 23:20

rebello95 reviewed Aug 5, 2019

View reviewed changes

library/objective-c/BUILD Outdated Show resolved Hide resolved

goaway force-pushed the ms-junr03/callbacks branch 2 times, most recently from 615a3be to c17fcc1 Compare August 5, 2019 23:31

rebello95 previously approved these changes Aug 5, 2019

View reviewed changes

goaway and others added 11 commits August 5, 2019 17:36

wip

1eaab7b

Signed-off-by: Mike Schore <mike.schore@gmail.com>

more wip

7558c25

Signed-off-by: Mike Schore <mike.schore@gmail.com>

builds without atomic

e765f48

Signed-off-by: Jose Nino <jnino@lyft.com>

fix atomic and convert to pure Objective-C

1e54af3

Signed-off-by: Mike Schore <mike.schore@gmail.com>

organization

90ea8e5

Signed-off-by: Mike Schore <mike.schore@gmail.com>

updates for feedback

f549b0b

Signed-off-by: Mike Schore <mike.schore@gmail.com>

clean up

9c6f799

Signed-off-by: Mike Schore <mike.schore@gmail.com>

init Envoy with engine type

75d2e05

Signed-off-by: Mike Schore <mike.schore@gmail.com>

format

a049c2f

Signed-off-by: Mike Schore <mike.schore@gmail.com>

add todo

4d650d8

Signed-off-by: Mike Schore <mike.schore@gmail.com>

format

cbd65cd

Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway dismissed rebello95’s stale review via cbd65cd August 6, 2019 00:36

goaway force-pushed the ms-junr03/callbacks branch from c17fcc1 to cbd65cd Compare August 6, 2019 00:36

goaway added 3 commits August 6, 2019 09:46

move definitions

03d6c27

Signed-off-by: Mike Schore <mike.schore@gmail.com>

move missing header deps

d61dbb6

Signed-off-by: Mike Schore <mike.schore@gmail.com>

add nolint

9c58383

Signed-off-by: Mike Schore <mike.schore@gmail.com>

rebello95 mentioned this pull request Aug 7, 2019

ios: finish bridging between ObjC/Swift #312

Merged

keith reviewed Aug 7, 2019

View reviewed changes

bazel/swift_static_framework.bzl Outdated Show resolved Hide resolved

goaway added 3 commits August 7, 2019 11:12

manually create bridging header

b9999be

Signed-off-by: Mike Schore <mike.schore@gmail.com>

fix types in header utility test

c5a1378

Signed-off-by: Mike Schore <mike.schore@gmail.com>

fix header utility test

7430631

Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway mentioned this pull request Aug 7, 2019

envoy: update upstream for onComplete callback #318

Merged

goaway added 6 commits August 7, 2019 15:34

fix signature of send_metadata

820ae71

Signed-off-by: Mike Schore <mike.schore@gmail.com>

fix remaining missing symbol from Swift tests

5d61036

Signed-off-by: Mike Schore <mike.schore@gmail.com>

collapse sources

39e1798

Signed-off-by: Mike Schore <mike.schore@gmail.com>

reduce scope of diff

dc9da5d

Signed-off-by: Mike Schore <mike.schore@gmail.com>

get rid of EnvoyEngine protocol

42de9c3

Signed-off-by: Mike Schore <mike.schore@gmail.com>

cleanup some aliases we may not use anyways

4da7f3c

Signed-off-by: Mike Schore <mike.schore@gmail.com>

lint

3bba7d3

Signed-off-by: Mike Schore <mike.schore@gmail.com>

rebello95 approved these changes Aug 9, 2019

View reviewed changes

goaway merged commit da4d4d6 into master Aug 9, 2019

rebello95 mentioned this pull request Aug 12, 2019

objc: convert EnvoyEngine into a protocol #324

Merged

junr03 deleted the ms-junr03/callbacks branch August 20, 2019 20:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add native-platform callback implementation #316

Add native-platform callback implementation #316

goaway commented Aug 1, 2019

rebello95 left a comment

rebello95 Aug 3, 2019

goaway Aug 5, 2019

rebello95 Aug 3, 2019

goaway Aug 5, 2019

rebello95 Aug 5, 2019

goaway Aug 5, 2019

rebello95 Aug 5, 2019

rebello95 Aug 5, 2019

goaway Aug 5, 2019

goaway Aug 5, 2019

rebello95 Aug 5, 2019

goaway Aug 5, 2019

rebello95 left a comment

rebello95 commented Aug 9, 2019

goaway commented Aug 9, 2019

rebello95 commented Aug 9, 2019

Add native-platform callback implementation #316

Add native-platform callback implementation #316

Conversation

goaway commented Aug 1, 2019

rebello95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rebello95 left a comment

Choose a reason for hiding this comment

rebello95 commented Aug 9, 2019

goaway commented Aug 9, 2019

rebello95 commented Aug 9, 2019