-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Conversation
`/api/v0/cat` calls `ipfs.cat`, but `ipfs.cat` returns a buffer of file output. This changes `/api/v0/cat` to actually stream output by calling `ipfs.catPullStream` instead. It uses `pull-pushable` in order to catch an initial error in the stream before it starts flowing and instead return a plain JSON response with an appropriate HTTP status. This isn't ideal as it means there's no backpressure - if the consumer doesn't consume fast enough then data will start to get buffered into memory. However this is significantly better than buffering _everything_ into memory before replying. License: MIT Signed-off-by: Alan Shaw <[email protected]>
🤯🤯🤯🤯🤯🤯 |
const content = file.content | ||
if (!content && file.type === 'dir') { | ||
|
||
if (!file.content && file.type === 'dir') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These conditions might be simpler as:
if (!file.content && file.type === 'dir') { | |
if (file.type !== 'file') { | |
return d.abort(new Error('this dag node was not a file') | |
} | |
if (!file.content) { | |
return d.abort(new Error('this dag node had no content') | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that companion depends on the error message "this dag node is a directory" (it is the same for go ipfs) - it is cheaper to call cat and receive this error message than do two calls, one to figure it out. I'd rather not introduce another breaking change for this release (it's really breaky already)!
The interesting bit is a null
file.content
. When type is file
, is that possible? I'll add this test in because that could break the pull pipeline if we resolve the deferred with something that isn't a pull stream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. I guess once all the errors have codes we can stop depending on message content.
If file.type === 'file'
then file.content
should always have a value. It's weird that there is a check for that in the first place 🤷♂️.
License: MIT Signed-off-by: Alan Shaw <[email protected]>
Merging as test failures seem to be because of unrelated IPNS tests. |
/api/v0/cat
callsipfs.cat
, butipfs.cat
returns a buffer of file output. This changes/api/v0/cat
to actually stream output by callingipfs.catPullStream
instead.It uses
pull-pushable
in order to catch an initial error in the stream before it starts flowing and instead return a plain JSON response with an appropriate HTTP status.This isn't ideal as it means there's no backpressure - if the consumer doesn't consume fast enough then data will start to get buffered into memory. However this is significantly better than buffering everything into memory before replying.
This reduces memory usage and time to first byte significantly. e.g.
resolves #1755