stream intel: wire iOS FinalStreamIntel for parity with jvm/android (#…

…1964)

Description: I've changed up my approach here. Previously, I mirrored Android/core APIs by having a separate struct for FinalStreamIntel. Upon further consideration, I feel it's a significant improvement to the API's usability and future flexibility to have FinalStreamIntel be a true superset (and derivative) of StreamIntel. With this change the terminal callbacks - onComplete (previously not exposed in the public API), onCancel, and onError contain a FinalStreamIntel object, and other callbacks contain the prior StreamIntel subset.
Risk Level: Moderate
Testing: Unit, Integration

Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: JP Simard <jp@jpsim.com>

mobile/examples/objective-c/hello_world/ViewController.m

@@ -103,7 +103,7 @@ - (void)performRequest {
 
     [weakSelf addResponseMessage:message headerMessage:headerMessage error:nil];
   }];
-  [prototype setOnErrorWithClosure:^(EnvoyError *error, StreamIntel *ignored) {
+  [prototype setOnErrorWithClosure:^(EnvoyError *error, FinalStreamIntel *ignored) {
     // TODO: expose attemptCount. https://github.com/envoyproxy/envoy-mobile/issues/823
     NSString *message =
         [NSString stringWithFormat:@"failed within Envoy library %@", error.message];

mobile/examples/swift/hello_world/AsyncDemoFilter.swift

@@ -59,7 +59,9 @@ final class AsyncDemoFilter: AsyncResponseFilter {
     return .resumeIteration(headers: builder.build(), data: data, trailers: trailers)
   }
 
-  func onError(_ error: EnvoyError, streamIntel: StreamIntel) {}
+  func onError(_ error: EnvoyError, streamIntel: FinalStreamIntel) {}
 
-  func onCancel(streamIntel: StreamIntel) {}
+  func onCancel(streamIntel: FinalStreamIntel) {}
+
+  func onComplete(streamIntel: FinalStreamIntel) {}
 }

mobile/examples/swift/hello_world/BufferDemoFilter.swift

@@ -40,7 +40,9 @@ final class BufferDemoFilter: ResponseFilter {
     return .resumeIteration(headers: builder.build(), data: self.body, trailers: trailers)
   }
 
-  func onError(_ error: EnvoyError, streamIntel: StreamIntel) {}
+  func onError(_ error: EnvoyError, streamIntel: FinalStreamIntel) {}
 
-  func onCancel(streamIntel: StreamIntel) {}
+  func onCancel(streamIntel: FinalStreamIntel) {}
+
+  func onComplete(streamIntel: FinalStreamIntel) {}
 }

mobile/examples/swift/hello_world/DemoFilter.swift

@@ -25,7 +25,9 @@ struct DemoFilter: ResponseFilter {
     return .continue(trailers: trailers)
   }
 
-  func onError(_ error: EnvoyError, streamIntel: StreamIntel) {}
+  func onError(_ error: EnvoyError, streamIntel: FinalStreamIntel) {}
 
-  func onCancel(streamIntel: StreamIntel) {}
+  func onCancel(streamIntel: FinalStreamIntel) {}
+
+  func onComplete(streamIntel: FinalStreamIntel) {}
 }

mobile/library/kotlin/io/envoyproxy/envoymobile/StreamPrototype.kt

@@ -61,6 +61,7 @@ open class StreamPrototype(private val engine: EnvoyEngine) {
 
   /**
    * Specify a callback for when response headers are received by the stream.
+   * If `endStream` is `true`, the stream is complete, pending an onComplete callback.
    *
    * @param closure Closure which will receive the headers and flag indicating if the stream
    * is headers-only.
@@ -75,7 +76,7 @@ open class StreamPrototype(private val engine: EnvoyEngine) {
 
   /**
    * Specify a callback for when a data frame is received by the stream.
-   * If `endStream` is `true`, the stream is complete.
+   * If `endStream` is `true`, the stream is complete, pending an onComplete callback.
    *
    * @param closure Closure which will receive the data and flag indicating whether this
    * is the last data frame.
@@ -90,7 +91,7 @@ open class StreamPrototype(private val engine: EnvoyEngine) {
 
   /**
    * Specify a callback for when trailers are received by the stream.
-   * If the closure is called, the stream is complete.
+   * If the closure is called, the stream is complete, pending an onComplete callback.
    *
    * @param closure Closure which will receive the trailers.
    * @return This stream, for chaining syntax.

mobile/library/kotlin/io/envoyproxy/envoymobile/filters/ResponseFilter.kt

@@ -50,6 +50,7 @@ interface ResponseFilter : Filter {
   /**
    * Called at most once when an error within Envoy occurs.
    *
+   * Only one of onError, onCancel, or onComplete will be called per stream.
    * This should be considered a terminal state, and invalidates any previous attempts to
    * `stopIteration{...}`.
    *
@@ -62,6 +63,7 @@ interface ResponseFilter : Filter {
   /**
    * Called at most once when the client cancels the stream.
    *
+   * Only one of onError, onCancel, or onComplete will be called per stream.
    * This should be considered a terminal state, and invalidates any previous attempts to
    * `stopIteration{...}`.
    *
@@ -71,8 +73,9 @@ interface ResponseFilter : Filter {
   fun onCancel(streamIntel: StreamIntel, finalStreamIntel: FinalStreamIntel)
 
 /**
-   * Called at most once when the stream is complete.
+   * Called at most once when the stream completes gracefully.
    *
+   * Only one of onError, onCancel, or onComplete will be called per stream.
    * This should be considered a terminal state, and invalidates any previous attempts to
    * `stopIteration{...}`.
    *

mobile/library/objective-c/EnvoyEngine.h

@@ -17,6 +17,9 @@ typedef NSDictionary<NSString *, NSString *> EnvoyEvent;
 /// Contains internal HTTP stream metrics, context, and other details.
 typedef envoy_stream_intel EnvoyStreamIntel;
 
+// Contains one time HTTP stream metrics, context, and other details.
+typedef envoy_final_stream_intel EnvoyFinalStreamIntel;
+
 #pragma mark - EnvoyHTTPCallbacks
 
 /// Interface that can handle callbacks from an HTTP stream.
@@ -27,14 +30,17 @@ typedef envoy_stream_intel EnvoyStreamIntel;
  */
 @property (nonatomic, assign) dispatch_queue_t dispatchQueue;
 
+// Formatting for block properties is inconsistent and not configurable.
+// clang-format off
+
 /**
  * Called when all headers get received on the async HTTP stream.
  * @param headers the headers received.
  * @param endStream whether the response is headers-only.
  * @param streamIntel internal HTTP stream metrics, context, and other details.
  */
-@property (nonatomic, copy) void (^onHeaders)
-    (EnvoyHeaders *headers, BOOL endStream, EnvoyStreamIntel streamIntel);
+@property (nonatomic, copy) void (^onHeaders)(
+    EnvoyHeaders *headers, BOOL endStream, EnvoyStreamIntel streamIntel);
 
 /**
  * Called when a data frame gets received on the async HTTP stream.
@@ -43,36 +49,49 @@ typedef envoy_stream_intel EnvoyStreamIntel;
  * @param endStream whether the data is the last data frame.
  * @param streamIntel internal HTTP stream metrics, context, and other details.
  */
-@property (nonatomic, copy) void (^onData)
-    (NSData *data, BOOL endStream, EnvoyStreamIntel streamIntel);
+@property (nonatomic, copy) void (^onData)(
+    NSData *data, BOOL endStream, EnvoyStreamIntel streamIntel);
 
-// clang-format off
 /**
  * Called when all trailers get received on the async HTTP stream.
  * Note that end stream is implied when on_trailers is called.
  * @param trailers the trailers received.
  * @param streamIntel internal HTTP stream metrics, context, and other details.
  */
-@property (nonatomic, copy) void (^onTrailers)
-    (EnvoyHeaders *trailers, EnvoyStreamIntel streamIntel);
-// clang-format on
+@property (nonatomic, copy) void (^onTrailers)(
+    EnvoyHeaders *trailers, EnvoyStreamIntel streamIntel);
 
 /**
  * Called when the async HTTP stream has an error.
  * @param streamIntel internal HTTP stream metrics, context, and other details.
+ * @param finalStreamIntel one time HTTP stream metrics, context, and other details.
  */
-@property (nonatomic, copy) void (^onError)
-    (uint64_t errorCode, NSString *message, int32_t attemptCount, EnvoyStreamIntel streamIntel);
+@property (nonatomic, copy) void (^onError)(
+    uint64_t errorCode, NSString *message, int32_t attemptCount, EnvoyStreamIntel streamIntel,
+    EnvoyFinalStreamIntel finalStreamIntel);
 
 /**
  * Called when the async HTTP stream is canceled.
  * Note this callback will ALWAYS be fired if a stream is canceled, even if the request and/or
  * response is already complete. It will fire no more than once, and no other callbacks for the
  * stream will be issued afterwards.
  * @param streamIntel internal HTTP stream metrics, context, and other details.
+ * @param finalStreamIntel one time HTTP stream metrics, context, and other details.
+ */
+@property (nonatomic, copy) void (^onCancel)(
+    EnvoyStreamIntel streamIntel, EnvoyFinalStreamIntel finalStreamIntel);
+
+/**
+ * Final call made when an HTTP stream is closed gracefully.
+ * Note this may already be inferred from a prior callback with endStream=TRUE, and this only needs
+ * to be handled if information from finalStreamIntel is desired.
+ * @param streamIntel internal HTTP stream metrics, context, and other details.
+ * @param finalStreamIntel one time HTTP stream metrics, context, and other details.
  */
-@property (nonatomic, copy) void (^onCancel)(EnvoyStreamIntel streamIntel);
+@property (nonatomic, copy) void (^onComplete)(
+    EnvoyStreamIntel streamIntel, EnvoyFinalStreamIntel finalStreamIntel);
 
+// clang-format on
 @end
 
 #pragma mark - EnvoyHTTPFilter
@@ -114,79 +133,86 @@ extern const int kEnvoyFilterResumeStatusResumeIteration;
 
 @interface EnvoyHTTPFilter : NSObject
 
+// Formatting for block properties is inconsistent and not configurable.
+// clang-format off
+
 /// Returns tuple of:
 /// 0 - NSNumber *,filter status
 /// 1 - EnvoyHeaders *, forward headers
-@property (nonatomic, copy) NSArray * (^onRequestHeaders)
-    (EnvoyHeaders *headers, BOOL endStream, EnvoyStreamIntel streamIntel);
+@property (nonatomic, copy) NSArray * (^onRequestHeaders)(
+    EnvoyHeaders *headers, BOOL endStream, EnvoyStreamIntel streamIntel);
 
 /// Returns tuple of:
 /// 0 - NSNumber *,filter status
 /// 1 - NSData *, forward data
 /// 2 - EnvoyHeaders *, optional pending headers
-@property (nonatomic, copy) NSArray * (^onRequestData)
-    (NSData *data, BOOL endStream, EnvoyStreamIntel streamIntel);
+@property (nonatomic, copy) NSArray * (^onRequestData)(
+    NSData *data, BOOL endStream, EnvoyStreamIntel streamIntel);
 
 /// Returns tuple of:
 /// 0 - NSNumber *,filter status
 /// 1 - EnvoyHeaders *, forward trailers
 /// 2 - EnvoyHeaders *, optional pending headers
 /// 3 - NSData *, optional pending data
-@property (nonatomic, copy) NSArray * (^onRequestTrailers)
-    (EnvoyHeaders *trailers, EnvoyStreamIntel streamIntel);
+@property (nonatomic, copy) NSArray * (^onRequestTrailers)(
+    EnvoyHeaders *trailers, EnvoyStreamIntel streamIntel);
 
 /// Returns tuple of:
 /// 0 - NSNumber *,filter status
 /// 1 - EnvoyHeaders *, forward headers
-@property (nonatomic, copy) NSArray * (^onResponseHeaders)
-    (EnvoyHeaders *headers, BOOL endStream, EnvoyStreamIntel streamIntel);
+@property (nonatomic, copy) NSArray * (^onResponseHeaders)(
+    EnvoyHeaders *headers, BOOL endStream, EnvoyStreamIntel streamIntel);
 
 /// Returns tuple of:
 /// 0 - NSNumber *,filter status
 /// 1 - NSData *, forward data
 /// 2 - EnvoyHeaders *, optional pending headers
-@property (nonatomic, copy) NSArray * (^onResponseData)
-    (NSData *data, BOOL endStream, EnvoyStreamIntel streamIntel);
+@property (nonatomic, copy) NSArray * (^onResponseData)(
+    NSData *data, BOOL endStream, EnvoyStreamIntel streamIntel);
 
 /// Returns tuple of:
 /// 0 - NSNumber *,filter status
 /// 1 - EnvoyHeaders *, forward trailers
 /// 2 - EnvoyHeaders *, optional pending headers
 /// 3 - NSData *, optional pending data
-@property (nonatomic, copy) NSArray * (^onResponseTrailers)
-    (EnvoyHeaders *trailers, EnvoyStreamIntel streamIntel);
+@property (nonatomic, copy)NSArray * (^onResponseTrailers)(
+    EnvoyHeaders *trailers, EnvoyStreamIntel streamIntel);
 
-@property (nonatomic, copy) void (^onCancel)(EnvoyStreamIntel streamIntel);
+@property (nonatomic, copy) void (^onCancel)(
+    EnvoyStreamIntel streamIntel, EnvoyFinalStreamIntel finalStreamIntel);
 
-@property (nonatomic, copy) void (^onError)
-    (uint64_t errorCode, NSString *message, int32_t attemptCount, EnvoyStreamIntel streamIntel);
+@property (nonatomic, copy) void (^onError)(
+    uint64_t errorCode, NSString *message, int32_t attemptCount, EnvoyStreamIntel streamIntel,
+    EnvoyFinalStreamIntel finalStreamIntel);
 
-@property (nonatomic, copy) void (^setRequestFilterCallbacks)
-    (id<EnvoyHTTPFilterCallbacks> callbacks);
+@property (nonatomic, copy) void (^onComplete)(
+    EnvoyStreamIntel streamIntel, EnvoyFinalStreamIntel finalStreamIntel);
+
+@property (nonatomic, copy) void (^setRequestFilterCallbacks)(
+    id<EnvoyHTTPFilterCallbacks> callbacks);
 
-// clang-format off
 /// Returns tuple of:
 /// 0 - NSNumber *,filter status
 /// 1 - EnvoyHeaders *, optional pending headers
 /// 2 - NSData *, optional pending data
 /// 3 - EnvoyHeaders *, optional pending trailers
-@property (nonatomic, copy) NSArray * (^onResumeRequest)
-    (EnvoyHeaders *_Nullable headers, NSData *_Nullable data, EnvoyHeaders *_Nullable trailers,
-     BOOL endStream, EnvoyStreamIntel streamIntel);
+@property (nonatomic, copy) NSArray * (^onResumeRequest)(
+    EnvoyHeaders *_Nullable headers, NSData *_Nullable data, EnvoyHeaders *_Nullable trailers,
+    BOOL endStream, EnvoyStreamIntel streamIntel);
 
-@property (nonatomic, copy) void (^setResponseFilterCallbacks)
-    (id<EnvoyHTTPFilterCallbacks> callbacks);
+@property (nonatomic, copy) void (^setResponseFilterCallbacks)(
+    id<EnvoyHTTPFilterCallbacks> callbacks);
 
 /// Returns tuple of:
 /// 0 - NSNumber *,filter status
 /// 1 - EnvoyHeaders *, optional pending headers
 /// 2 - NSData *, optional pending data
 /// 3 - EnvoyHeaders *, optional pending trailers
-@property (nonatomic, copy) NSArray * (^onResumeResponse)
-    (EnvoyHeaders *_Nullable headers, NSData *_Nullable data, EnvoyHeaders *_Nullable trailers,
-     BOOL endStream, EnvoyStreamIntel streamIntel);
-// clang-format on
+@property (nonatomic, copy) NSArray * (^onResumeResponse)(
+    EnvoyHeaders *_Nullable headers, NSData *_Nullable data, EnvoyHeaders *_Nullable trailers,
+    BOOL endStream, EnvoyStreamIntel streamIntel);
 
+// clang-format on
 @end
 
 #pragma mark - EnvoyHTTPFilterFactory

mobile/library/objective-c/EnvoyEngineImpl.m

@@ -332,6 +332,20 @@ static void ios_http_filter_set_response_callbacks(envoy_http_filter_callbacks c
   }
 }
 
+static void ios_http_filter_on_complete(envoy_stream_intel stream_intel,
+                                        envoy_final_stream_intel final_stream_intel,
+                                        const void *context) {
+  // This code block runs inside the Envoy event loop. Therefore, an explicit autoreleasepool block
+  // is necessary to act as a breaker for any Objective-C allocation that happens.
+  @autoreleasepool {
+    EnvoyHTTPFilter *filter = (__bridge EnvoyHTTPFilter *)context;
+    if (filter.onComplete == nil) {
+      return;
+    }
+    filter.onComplete(stream_intel, final_stream_intel);
+  }
+}
+
 static void ios_http_filter_on_cancel(envoy_stream_intel stream_intel,
                                       envoy_final_stream_intel final_stream_intel,
                                       const void *context) {
@@ -342,7 +356,7 @@ static void ios_http_filter_on_cancel(envoy_stream_intel stream_intel,
     if (filter.onCancel == nil) {
       return;
     }
-    filter.onCancel(stream_intel);
+    filter.onCancel(stream_intel, final_stream_intel);
   }
 }
 
@@ -363,7 +377,8 @@ static void ios_http_filter_on_error(envoy_error error, envoy_stream_intel strea
                                                     encoding:NSUTF8StringEncoding];
 
     release_envoy_error(error);
-    filter.onError(error.error_code, errorMessage, error.attempt_count, stream_intel);
+    filter.onError(error.error_code, errorMessage, error.attempt_count, stream_intel,
+                   final_stream_intel);
   }
 }
 
@@ -457,6 +472,8 @@ - (int)registerFilterFactory:(EnvoyHTTPFilterFactory *)filterFactory {
   api->on_resume_request = ios_http_filter_on_resume_request;
   api->set_response_callbacks = ios_http_filter_set_response_callbacks;
   api->on_resume_response = ios_http_filter_on_resume_response;
+  // TODO(goaway) HTTP filter on_complete not currently implemented.
+  // api->on_complete = ios_http_filter_on_complete;
   api->on_cancel = ios_http_filter_on_cancel;
   api->on_error = ios_http_filter_on_error;
   api->release_filter = ios_http_filter_release;

mobile/library/objective-c/EnvoyHTTPStreamImpl.m

@@ -68,7 +68,10 @@
   EnvoyHTTPCallbacks *callbacks = c->callbacks;
   EnvoyHTTPStreamImpl *stream = c->stream;
   dispatch_async(callbacks.dispatchQueue, ^{
-    // TODO: If the callback queue is not serial, clean up is not currently thread-safe.
+    if (callbacks.onComplete) {
+      callbacks.onComplete(stream_intel, final_stream_intel);
+    }
+
     assert(stream);
     [stream cleanUp];
   });
@@ -90,7 +93,7 @@
   EnvoyHTTPStreamImpl *stream = c->stream;
   dispatch_async(callbacks.dispatchQueue, ^{
     if (callbacks.onCancel) {
-      callbacks.onCancel(stream_intel);
+      callbacks.onCancel(stream_intel, final_stream_intel);
     }
 
     // TODO: If the callback queue is not serial, clean up is not currently thread-safe.
@@ -111,7 +114,8 @@
                                                         length:error.message.length
                                                       encoding:NSUTF8StringEncoding];
       release_envoy_error(error);
-      callbacks.onError(error.error_code, errorMessage, error.attempt_count, stream_intel);
+      callbacks.onError(error.error_code, errorMessage, error.attempt_count, stream_intel,
+                        final_stream_intel);
     }
 
     // TODO: If the callback queue is not serial, clean up is not currently thread-safe.

mobile/library/swift/BUILD

@@ -12,6 +12,7 @@ swift_library(
         "EngineBuilder.swift",
         "EngineImpl.swift",
         "EnvoyError.swift",
+        "FinalStreamIntel.swift",
         "Headers.swift",
         "HeadersBuilder.swift",
         "LogLevel.swift",