Client.putObject doesnt correctly detect destroyed streams

[MaxNoetzold]

Observed

The Client.putObject sometimes (often) stalls when given a destroyed stream.

Expected

The Client.putObject throws an error if a non-readable stream is found.

Notes

In my opinion, there are two problems in the Client.putObject function regarding stream error handling.

First: The isReadableStream check is not sufficient. If a destroyed stream gets checked by isReadableStream, it will return true which is wrong. In that case the function putObject will stall forever, because it tries to read a non-readable stream. Possible fix: Use isReadable from the nodejs library.

Second: Even if the isReadableStream check would work, it would be possible that the stream is destroyed after the check but before the Error listener. In that case the library would stall forever again. Note: This was not a problem in older versions before the async calls findUploadId, initiateNewMultipartUpload and listParts got added, since the node eventloop helped us here. However, now this is a problem. Possible fix: Listen to errors earlier (which might be hard to enforce for future changes). Therefore, second option: Use the isReadable check again.

[prakashsvmx]

yeah. we need safe way across LTS versions. feel free to send changes as PR.

[MaxNoetzold]

The docs state:

"This document presumes you have a working Node.js development environment, LTS versions v16, v18 or v20."

I read that as minio-js supporting those three LTS lines. I checked Node's isReadable implementation for those versions. isReadable was introduced in v16.14.0 and has remained effectively unchanged since. The only noteworthy differences in v18 are:

• stream[kIsDestroyed] check was added (so v18+ checks an internal symbol in addition to stream.destroyed).
• (!strict || (typeof obj.pause === 'function' && typeof obj.resume === 'function')) check was added

I copied the exact code with my comments at the bottom if you want to have a closer look.

In my opinion we could safely use the builtin isReadable function or copy paste their v18 code into the library and use it for all versions.

What do you think?

isReadable Code

// didnt change since v16
function isReadable(stream) {
  if (stream && stream[kIsReadable] != null) return stream[kIsReadable];
  if (typeof stream?.readable !== 'boolean') return null;
  if (isDestroyed(stream)) return false;
  return isReadableNodeStream(stream) &&
    stream.readable &&
    !isReadableFinished(stream);
}

function isDestroyed(stream) {
  if (!isNodeStream(stream)) return null;
  const wState = stream._writableState;
  const rState = stream._readableState;
  const state = wState || rState;
  // stream[kIsDestroyed] got added with v18
  return !!(stream.destroyed || stream[kIsDestroyed] || state?.destroyed);
}

function isReadableNodeStream(obj, strict = false) {
  return !!(
    obj &&
    typeof obj.pipe === 'function' &&
    typeof obj.on === 'function' &&
	// This () was added with v18
    (
      !strict ||
      (typeof obj.pause === 'function' && typeof obj.resume === 'function')
    ) &&
    (!obj._writableState || obj._readableState?.readable !== false) && 
    (!obj._writableState || obj._readableState)
  );
}
// didnt change since v16
function isReadableFinished(stream, strict) {
  if (!isReadableNodeStream(stream)) return null;
  const rState = stream._readableState;
  if (rState?.errored) return false;
  if (typeof rState?.endEmitted !== 'boolean') return null;
  return !!(
    rState.endEmitted ||
    (strict === false && rState.ended === true && rState.length === 0)
  );
}