Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stream.compose does not preserve 'readableObjectMode' from final stream to returned stream #46829

Closed
loganfsmyth opened this issue Feb 24, 2023 · 6 comments
Labels
stream Issues and PRs related to the stream subsystem.

Comments

@loganfsmyth
Copy link
Contributor

loganfsmyth commented Feb 24, 2023

Version

v19.7.0

Platform

No response

Subsystem

No response

What steps will reproduce the bug?

const stream = require("stream");

const s = stream.compose(
  stream.Readable.from("0 1 2 3 4"),
  new stream.Transform({
    readableObjectMode: true,
    transform: function (val, enc, callback) {
      for (const num of val.toString().split(" ")) {
        this.push(`${num}`);
      }
      callback();
    },
  })
);

s.on("readable", () => {
  let data;
  while ((data = s.read()) !== null) {
    console.log("read:", `${data}`);
  }
});
s.on("end", () => {
  console.log("done");
});

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior?

Should output

read: 0
read: 1
read: 2
read: 3
read: 4
done 

What do you see instead?

read: 01234
read: null
done

Additional information

No response

@loganfsmyth
Copy link
Contributor Author

I'm assuming this is just a typo on this line:

readableObjectMode: !!tail?.writableObjectMode,

@climba03003
Copy link
Contributor

climba03003 commented Feb 25, 2023

You should use .on('data') instead of .on('readable').
readable event and .read consume all the pending data at once.
So, the output is concatenated string for all 0, 1, 2, 3, 4.

https://nodejs.org/dist/latest-v18.x/docs/api/stream.html#readablereadsize

If the size argument is not specified, all of the data contained in the internal buffer will be returned.

@loganfsmyth
Copy link
Contributor Author

That's absolutely true for non-object-mode streams, but I'm confident that's not expected for an object mode stream. I think the code above just should be

-readableObjectMode: !!tail?.writableObjectMode, 
+readableObjectMode: !!tail?.readableObjectMode, 

@benjamingr
Copy link
Member

I don't understand, the transform stream needs to be both readable and writable in object mode. Otherwise the returned stream (the one the transform is writing into) isn't in object mode (the return stream isn't in writableObjectMode anyway).

The example works if you add writableObjectMode: true to the transform stream (or just objectMode: true instead of both).

@loganfsmyth
Copy link
Contributor Author

the transform stream needs to be both readable and writable in object mode

Can you clarify what you mean by that? I know my transform stream could be a writable object stream, but I don't think it's required or expected that anything that is readable-object-mode must be in writable-object-mode. In this case I'm expecting the transform to take in a byte stream and produce and object stream, essentially.

I'd expect that compose would return a stream that is:

  • writable-object-mode if the first argument is writable-object-mode
  • readable-object-mode if the last argument is readable-object-mode

@VoltrexKeyva VoltrexKeyva added the stream Issues and PRs related to the stream subsystem. label Feb 25, 2023
@rluvaton
Copy link
Member

rluvaton commented Apr 4, 2023

I agree with @loganfsmyth and it is expected IMO given that a Transform can get bytes and write object

"For Duplex streams, objectMode can be set exclusively for either the Readable or Writable side using the readableObjectMode and writableObjectMode options respectively."

From the Docs

This logs the data:

let readInByteWriteInObject = new Transform({
    // reading FROM you in object mode or not
    readableObjectMode: true,

    // writing TO you in object mode or not
    writableObjectMode: false,
    transform: function (value, enc, callback) {
        callback(null, {
            data: value
        });
    },
});



readInByteWriteInObject.on('data', (data) => {
    console.log(data);
});

readInByteWriteInObject.write('0 1 2 3 4')

but this does not:

let readInByteWriteInObject = new Transform({
    // reading FROM you in object mode or not
    readableObjectMode: true,

    // writing TO you in object mode or not
    writableObjectMode: false,
    transform: function (value, enc, callback) {
        callback(null, {
            data: value
        });
    },
});


const s = Readable.from("0 1 2 3 4", {objectMode: false})
    .compose(
        readInByteWriteInObject
    );

s.on('data', (data) => {
    console.log(data);
});

rluvaton added a commit to rluvaton/node that referenced this issue Apr 5, 2023
targos pushed a commit that referenced this issue May 2, 2023
Fixes: #46829

PR-URL: #47413
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
danielleadams pushed a commit that referenced this issue Jul 6, 2023
Fixes: #46829

PR-URL: #47413
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this issue Jul 6, 2023
Fixes: nodejs#46829

PR-URL: nodejs#47413
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
dvirtz added a commit to dvirtz/vscode-parquet-viewer that referenced this issue Sep 7, 2024
dvirtz added a commit to dvirtz/vscode-parquet-viewer that referenced this issue Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants