Swift testing #3431
Swift testing #3431
Conversation
swift/src/IceImpl/DispatchAdapter.mm
Outdated
| // For a batch request with multiple requests remaining in the input stream, we need to copy the encapsulation data | ||
| // because moving it isn't an option—subsequent requests still depend on the input stream. | ||
| // | ||
| // For oneway collocated requests, we also need to copy the encapsulation data because the input stream doesn't own |
There was a problem hiding this comment.
Let's take a two-way collocated request, where the client-side uses AMI. The OutputStream with the input args won't be released until after the response is delivered? What if the request gets canceled?
There was a problem hiding this comment.
Also, for a collocated request, we can't transfer ownership of the memory from the OutputStream args to the InputStream args because... ?
Retries maybe?
There was a problem hiding this comment.
Let's take a two-way collocated request, where the client-side uses AMI. The OutputStream with the input args won't be released until after the response is delivered? What if the request gets canceled?
I think this could be a problem too.
Retries maybe?
Yes. The retry would use the OutputStream
I wonder if a better fix would be to add a owned() method to Buffer/InputStream. Then if a dispatcher wants to move the InputStream to another thread like Swift does, it can check if the buffer is owned then just used std::move otherwise do a copy. We still have to deal with the special case for batch requests.
There was a problem hiding this comment.
Then if a dispatcher wants to move the InputStream to another thread like Swift does
I wouldn't do that because what we do in Swift is a corner case. A C++ dispatch pipeline executes in the same dispatch thread until it unmarshals the incoming args; we should not add APIs to support a more asynchronous model.
swift/src/IceImpl/DispatchAdapter.mm
Outdated
| // heap allocated vector as the remainder of the memory is needed for subsequent requests. | ||
| if (request.requestCount() > 1) | ||
| // The Swift side can asynchronously unmarshal the request, so we need to either move or copy the encapsulation | ||
| // data to ensure it remains alive until Swift completes unmarshaling. The data should then be freed once the |
There was a problem hiding this comment.
I would remove the second sentence. Obviously we don't leak this memory. It's also not ideal to keep it around until the dispatch finishes; ideally, we would release it immediately after unmarshaling the incoming args.
There was a problem hiding this comment.
we would release it immediately after unmarshaling the incoming args.
This would break collocation retries. If the dispatch throws a exception that can be retried.
This PR updates the C++ collocated request handler to adopt the output stream buffer, when creating the InputStream for dispatch.
As show in #3286 the unmarshalling can happen asynchronously in which case the OutputStream that owned the memory can be removed before than the request is dispatched.
-- EDIT --
The original fix didn't work well because OutputStream can be user after the first dispatch if the operation is retried.
The actual fix is to always copy the incoming encaps for collocated dispatch.