Update Objective-C on error/cancel to include final_stream_intel #1954
Conversation
Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
| @@ -74,14 +75,15 @@ | |||
| return NULL; | |||
| } | |||
|
|
|||
| static void *ios_on_cancel(envoy_stream_intel stream_intel, void *context) { | |||
| static void *ios_on_send_window_available(envoy_stream_intel stream_intel, void *context) { | |||
There was a problem hiding this comment.
Looks like this got switched up with ios_on_cancel.
There was a problem hiding this comment.
Renamed to ios_on_cancel_impl in 7af1f6e
There was a problem hiding this comment.
Instead, could you update on_cancel and just ignore the new arg? And then add a no-op on_send_window_available stub.
There was a problem hiding this comment.
I was under the impression that on_send_window_available was calling ios_on_cancel on purpose.
If that can be a no-op for now, then I'll do that. I didn't want to change behavior in this PR without that context.
There was a problem hiding this comment.
nit - since you're not using the parameters, you can skip naming them and just include the type.
There was a problem hiding this comment.
This is Objective-C though, not C++/Objective-C++, so I get this error if I omit the parameter name:
external/envoy_mobile/library/objective-c/EnvoyHTTPStreamImpl.m:66:54: warning: omitting the parameter name in a function definition is a C2x extension [-Wc2x-extensions]
envoy_final_stream_intel, void *context) {
^
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim
force-pushed
the
objc-final-stream-intel
branch
from
52b0eb5
to
7af1f6e
Compare
|
Thanks for jumping on this @jpsim! |
Signed-off-by: JP Simard <jp@jpsim.com>
| @@ -143,9 +150,9 @@ - (instancetype)initWithHandle:(envoy_stream_t)handle | |||
|
|
|||
| // Create native callbacks | |||
| // TODO(goaway) fix this up to call ios_on_send_window_available | |||
| envoy_http_callbacks native_callbacks = {ios_on_headers, ios_on_data, ios_on_metadata, | |||
| ios_on_trailers, ios_on_error, ios_on_complete, | |||
| ios_on_cancel, ios_on_cancel, context}; | |||
There was a problem hiding this comment.
This was a (awful) hack, because we needed a function with a valid signature. It would be better to use an explicit no-op callback for on_send_window_available until we have time to wire it through.
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim
force-pushed
the
objc-final-stream-intel
branch
from
78bf584
to
d5867ef
Compare
|
If there's nothing further, @goaway could you please merge when you get a chance so we can update our downstream consumers? |
Description: This is a followup to #1937. Risk Level: I believe these extra parameters are safe to ignore if we don't need to propagate them to the Objective-C interface, but would like someone else to confirm that. (Confirmed.) Testing: Example apps still work, and downstream (closed source) consumers now compile again. Signed-off-by: JP Simard <jp@jpsim.com>
Description: This is a followup to #1937.
Risk Level: I believe these extra parameters are safe to ignore if we don't need to propagate them to the Objective-C interface, but would like someone else to confirm that.
Testing: Example apps still work, and downstream (closed source) consumers now compile again.
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]