hubcio commented on code in PR #3691:
URL: https://github.com/apache/iggy/pull/3691#discussion_r3613054366
##########
foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/AsyncIggyTcpClient.java:
##########
@@ -217,6 +221,39 @@ public CompletableFuture<IdentityInfo> login() {
return usersClient.login(username.get(), password.get());
}
+ /**
+ * Sends a command code and payload and returns the raw response payload.
+ *
+ * <p>Session-control codes complete the returned future with an
invalid-command error.
+ *
+ * @param code the command code
+ * @param payload the command payload
+ * @return a future containing the raw response payload
+ * @throws IggyNotConnectedException if {@link #connect()} has not been
called
+ */
+ public CompletableFuture<byte[]> sendRawWithResponse(int code, byte[]
payload) {
+ if (isSessionControlCode(code)) {
+ return CompletableFuture.failedFuture(
+
IggyServerException.fromTcpResponse(INVALID_COMMAND_ERROR_CODE, new byte[0]));
+ }
+ if (connection == null) {
+ throw new IggyNotConnectedException();
+ }
+
+ return connection.send(code,
Unpooled.wrappedBuffer(payload)).thenApply(response -> {
Review Comment:
`Unpooled.wrappedBuffer(payload)` aliases the caller's array without copying
- the copy into the frame happens later in `IggyFrameEncoder.encode` on the
netty event loop, after the async channel acquire (plus an auth round-trip when
it kicks in). an async caller that reuses or mutates the array while the future
is pending silently corrupts the payload on the wire. all typed commands build
fresh buffers, so raw is the only path aliasing caller memory.
`Unpooled.copiedBuffer(payload)` fixes it. same line also NPEs on null payload
for non-guarded codes (guarded ones return the failed future before touching
it), so a null check wouldn't hurt either.
##########
foreign/go/client/tcp/tcp_core.go:
##########
@@ -305,6 +305,38 @@ func (c *IggyTcpClient) do(ctx context.Context, cmd
command.Command) ([]byte, er
return resp, err
}
+// SendRawWithResponse sends a command code and payload and returns the raw
response body.
+// Session-control codes return ierror.ErrInvalidCommand without writing to
the connection.
+func (c *IggyTcpClient) SendRawWithResponse(ctx context.Context, code uint32,
payload []byte) ([]byte, error) {
+ if isSessionControlCode(code) {
+ return nil, ierror.ErrInvalidCommand
+ }
+
+ bp := acquireRequestBuf()
+ defer releaseRequestBuf(bp)
+
+ buf := append((*bp)[:0], 0, 0, 0, 0, 0, 0, 0, 0)
Review Comment:
this hand-rolls the same frame layout as `encodeWireRequest` below - 8-byte
header, code at [4:8], length patched from the realized body length - and that
length-from-realized-bytes rule is exactly the desync invariant the comment
above `encodeWireRequest` warns about. it can't be reused directly since it
takes a `command.Command`, but a small shared helper (`frameRaw(buf, code,
body)`) used by both would keep that invariant in one place.
##########
foreign/node/src/wire/command-set.ts:
##########
@@ -225,4 +237,20 @@ export abstract class CommandAPI extends AbstractAPI {
constructor(c: ClientProvider) {
super(c);
}
-};
+
+ /**
+ * Sends a command code with a payload and returns the raw response payload.
+ * Session-control codes are rejected with an invalid-command error.
+ *
+ * @param code - Command code to send
+ * @param payload - Raw command payload
+ * @returns Raw response payload
+ */
+ async sendRawWithResponse(code: number, payload: Buffer): Promise<Buffer> {
+ if (SESSION_CONTROL_CODES.has(code))
+ throw responseError(code, INVALID_COMMAND_ERROR_CODE);
+
+ const response = await (await this.clientProvider()).sendCommand(code,
payload);
Review Comment:
same aliasing shape as the java async client: the caller's `payload` travels
by reference through the send queue (`sendCommand` enqueues it, the copy into
the wire frame only happens at dequeue in `serializeCommand`). when the queue
is busy or connect/auth is still pending, a caller mutating the buffer
mid-flight corrupts the outgoing frame. typed commands always serialize into
fresh buffers, so raw is the first path that puts a caller-owned buffer in the
queue. a defensive `Buffer.from(payload)` here would match the java fix.
##########
bdd/go/tests/suite_test.go:
##########
@@ -53,6 +53,16 @@ func TestFeatures(t *testing.T) {
},
})
}
+ if feature == "all" || feature == "raw_command" {
+ suites = append(suites, godog.TestSuite{
+ ScenarioInitializer: initRawCommandScenario,
+ Options: &godog.Options{
+ Format: "pretty",
+ Paths:
[]string{"../../scenarios/raw_command.feature"},
+ TestingT: t,
Review Comment:
the two suites above got `Strict: true` (#3681) so undefined steps fail the
run - this new suite doesn't have it, so a typo'd step pattern here would
report undefined and still pass. add `Strict: true` for consistency.
##########
foreign/cpp/src/client.rs:
##########
@@ -1129,6 +1130,19 @@ impl Client {
Ok(())
})
}
+
+ /// Sends a command code and payload and returns the raw response bytes.
+ /// Session-control codes return an invalid-command error.
+ pub fn send_raw_with_response(&self, code: u32, payload: Vec<u8>) ->
Result<Vec<u8>, String> {
+ RUNTIME.block_on(async {
+ let response = self
+ .inner
+ .send_binary_request(code, Bytes::from(payload))
+ .await
+ .map_err(|error| format!("Could not send raw command '{code}':
{error}"))?;
+ Ok(response.to_vec())
Review Comment:
`response.to_vec()` always copies. `Vec::from(response)` reclaims the
allocation with no copy, since this `Bytes` comes uniquely from the tcp read
path (`BytesMut::freeze`). cold path so no urgency, just a free win. same
pattern in the php client's `send_raw_with_response`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
