fix: handle stream end properly in readChunk to avoid hanging on deferred-length uploads(Fixes #779)

docs/contributing.md

@@ -29,7 +29,8 @@ Tests are implemented using Jasmine and can be found in the `test/` directory. T
 
 - To run the tests inside **Node.js** use the command `yarn run test-node`.
 - To run the tests inside **Puppeteer** (an automated browser) use the command `yarn run test-puppeteer`.
-- To run the tests in your browser open `test/SpecRunner.html` in a browser and you should see a visual representation of the test results. No web server is required, you can open `SpecRunner.html` using the `file:///` protocol.
+- To run the tests in your browser open `test/SpecRunner.html` in a browser, and you should see a visual representation of the 
+  test results. No web server is required, you can open `SpecRunner.html` using the `file:///` protocol.
 - To run the tests on BrowserStack's cloud testing infrastructure use `yarn run test-browserstack`. Before using this command, you have to set up your BrowserStack account by filling the `BROWSERSTACK_USERNAME` and `BROWSERSTACK_KEY` variables or else the command will fail.
 
 Also note that you have to rebuild the library before you run the tests. So either you automatically let `yarn run watch` listen for file changes and automatically rebuild the needed artifacts, or you run `yarn run build` on your own before you execute the tests.

lib/node/sources/NodeStreamFileSource.ts

@@ -13,7 +13,7 @@ import type { FileSource } from '../../options.js'
 async function readChunk(stream: Readable, size: number) {
   return new Promise<Buffer>((resolve, reject) => {
     const onError = (err: Error) => {
-      stream.off('readable', onReadable)
+      cleanup()
       reject(err)
     }
 
@@ -22,15 +22,25 @@ async function readChunk(stream: Readable, size: number) {
       const chunk = stream.read(size)
 
       if (chunk != null) {
-        stream.off('error', onError)
-        stream.off('readable', onReadable)
-
+        cleanup()
         resolve(chunk)
       }
     }
 
+    const onEnd = () => {
+      cleanup()
+      resolve(Buffer.alloc(0))
+    }
+
+    const cleanup = () => {
+      stream.off('error', onError)
+      stream.off('readable', onReadable)
+      stream.off('end', onEnd)
+    }
+
     stream.once('error', onError)
     stream.on('readable', onReadable)
+    stream.once('end', onEnd)
   })
 }
 

test/spec/test-node-specific.js

@@ -106,6 +106,19 @@ describe('tus', () => {
         await expectHelloWorldUpload(input, options)
       })
 
+      it('should upload with deferred length and chunkSize divides size exactly', async () => {
+        const input = new stream.PassThrough()
+        const options = {
+          httpStack: new TestHttpStack(),
+          endpoint: '/uploads',
+          chunkSize: 7,
+          uploadLengthDeferred: true,
+        }
+
+        input.end('my hello WORLD')
+        await expectHelloWorldUploadWithDeferred(input, options)
+      })
+
       it('should throw an error if the source provides less data than uploadSize', async () => {
         const input = new stream.PassThrough()
         input.end('hello world')
@@ -561,3 +574,56 @@ async function expectHelloWorldUpload(input, options) {
 
   await options.onSuccess.toBeCalled()
 }
+
+//TODO: duplicates some logic from similar tests — can be refactored in a follow-up for maintainability
+async function expectHelloWorldUploadWithDeferred(input, options) {
+  options.httpStack = new TestHttpStack()
+  options.onSuccess = waitableFunction('onSuccess')
+
+  const upload = new Upload(input, options)
+  upload.start()
+
+  let req = await options.httpStack.nextRequest()
+  expect(req.url).toBe('/uploads')
+  expect(req.method).toBe('POST')
+  expect(req.requestHeaders['Upload-Length']).toBe(undefined)
+  expect(req.requestHeaders['Upload-Defer-Length']).toBe('1')
+  req.respondWith({
+    status: 201,
+    responseHeaders: { Location: '/uploads/blargh' },
+  })
+
+  req = await options.httpStack.nextRequest()
+  expect(req.url).toBe('/uploads/blargh')
+  expect(req.method).toBe('PATCH')
+  expect(req.requestHeaders['Upload-Offset']).toBe('0')
+  expect(req.bodySize).toBe(7)
+  req.respondWith({
+    status: 204,
+    responseHeaders: { 'Upload-Offset': '7' },
+  })
+
+  req = await options.httpStack.nextRequest()
+  expect(req.url).toBe('/uploads/blargh')
+  expect(req.method).toBe('PATCH')
+  expect(req.requestHeaders['Upload-Offset']).toBe('7')
+  expect(req.requestHeaders['Upload-Length']).toBe(undefined)
+  expect(req.bodySize).toBe(7)
+  req.respondWith({
+    status: 204,
+    responseHeaders: { 'Upload-Offset': '14' },
+  })
+
+  req = await options.httpStack.nextRequest()
+  expect(req.url).toBe('/uploads/blargh')
+  expect(req.method).toBe('PATCH')
+  expect(req.requestHeaders['Upload-Offset']).toBe('14')
+  expect(req.requestHeaders['Upload-Length']).toBe('14')
+  expect(req.bodySize).toBe(0)
+  req.respondWith({
+    status: 204,
+    responseHeaders: { 'Upload-Offset': '14' },
+  })
+
+  await options.onSuccess.toBeCalled()
+}