Repository navigation
Streaming compatibility with archiver #671
Description
Hi,
I'm trying to use sharp's streaming capability in tandem with archiver to resize and zip multiple images and stream out to a consumer. I can get each library to work on their own, but not together on any more than a single image.
Here's some example code:
const fs = require('fs');
const archiver = require('archiver');
const sharp = require('sharp');
//const imagemagick = require('imagemagick-stream');
const request = require('request');
function resizeAndZip() {
const imageStreams = [
request('http://i.stack.imgur.com/tKsDb.png'),
request('http://i.stack.imgur.com/EdUwb.png'),
request('http://i.stack.imgur.com/5d55j.png')
];
const zip = archiver('zip');
zip.on('entry', (entry) => console.log(`${entry.name} successfully appended to zip`));
imageStreams.map((stream) => {
const resize = sharp().resize(100, 100);
// const resize = imagemagick().resize('100x100');
return stream.pipe(resize);
})
.forEach((resizedImageStream, i) => {
zip.append(resizedImageStream, { name: `${i}.png` });
});
zip.finalize();
return zip;
}
resizeAndZip().pipe(fs.createWriteStream(__dirname + '/output.zip'));In the example you can see that I've also imported imagemagick-stream. If this library is used as the resizer instead of sharp everything works fine. I'm not sure whether this is a compatibility problem or if I'm using sharp incorrectly.
Activity
lovell commentedon Jan 6, 2017
Hello, I think the
resizedImageStreaminstances have yet to enter flowing mode. This happens automagically when something elsepipes data out, butarchiverdoesn't appear to do this for you.Something like the following should work:
Perhaps sharp's constructor jsdoc should include this information? I'd be happy to accept a PR if you'd like to make the docs clearer.
danhaller commentedon Jan 9, 2017
Thanks for the reply. Adding
resumeis definitely an improvement, the zip file now completes with all the files. It appears that most of the files are corrupt, however, when the archive is unzipped. I'm trying not to get too stackoverflow here but is this likely to be asharpissue or anarchiverissue?I'll definitely add a PR if you think it would be useful. Perhaps it would be more useful in
archiverthough, as I imagine most people using sharp will be usingpipe?lovell commentedon Jan 9, 2017
I can partially recreate this - the first two images in the zip file are OK but the third/last image has zero length.
To investigate further we'd need to look at the internals of the archiver module to see what custom logic it uses to handle a ReadableStream instance vs what Node gives you for "free" when using
pipe.lovell commentedon Jan 17, 2017
I think I might have identified a race condition where the Writable side of a
sharpinstance can emit the "finish" event before the Readable side has started flowing. My next step is to isolate it to a repeatable test case.(I had a quick look at the
archivercode and it does callpipeinternally in a sequential, queued manner. My advice toresumea Readable stream before callingzip.appendis probably wrong.)Ensure Readable can start flowing after Writable finish #671
lovell commentedon Jan 22, 2017
Commit d8df503 adds a test and the fix for the case where the Readable side of the Duplex Stream starts flowing only after the Writable side of the Duplex Stream has emitted the "finish" event. Thanks for reporting this.
sinegovsky-ivan commentedon Jan 27, 2017
The same issue with fix from "Commit d8df503" - 90% images have zero size :(
lovell commentedon Jan 27, 2017
@danhaller Have you been able to verify the proposed fix?
danhaller commentedon Jan 27, 2017
@lovell Thanks for investigating this and sorry I haven't got back to you sooner - busy week. I've just tried the latest commit with the code example above and I'm still seeing the same problem unfortunately - the first two images are zipped correctly but any others are not.
lovell commentedon Jan 27, 2017
Do you still see the problem if you remove the
finalizecall fromresizeAndZipand instead call it after you've started piping data to the filesystem?danhaller commentedon Jan 27, 2017
Ah I forgot to remove the
resume. After removing that it works correctly. I've also tried it out in the actual service that I'm writing and it works there too.Thanks again for fixing.
lovell commentedon Jan 27, 2017
Thanks @danhaller.
@sinegovsky-ivan if you're still having problems, please open a new issue with a code sample that fails, preferably with error handling/logging on the streams.
sinegovsky-ivan commentedon Jan 28, 2017
I also removed "resume" and all ok now.
Thank you for your work!
rn4391 commentedon Feb 7, 2017
If this is complete and working fine, then 0.17.2 can be released?
lovell commentedon Feb 11, 2017
v0.17.2 now available via npm, thanks for reporting this!