Add the `Expiration` extension, implement it in `FileStore` #320

bypie5 · 2022-10-26T03:13:27Z

Server owners to configure expiration time with expirationPeriodMintues property in FileStoreOptions. Then, the server owner can periodically invoke Server::cleanUpExpiredUploads() to remove uploads that are both expired AND incomplete.

Some linters (or personal preference) remove `async` from functions if there are no `await` or return a `Promise` object. This, with some bad errors management in Fastify, has lead me to two full week of head banging. With the sync signature, it gets more explicit that the body parsing always succeed by doing nothing. Note: `null` is needed due to how Typescript types are defined, for regular Javascript just calling `done()` should works.

bypie5 · 2022-10-26T03:17:59Z

Hello, I closed a similar PR a few days ago because the 1.x branch was not yet converted to typescript. Now that #308 has been merged in, I have reopened this PR with some minor changes.

Murderlon · 2022-10-31T16:22:00Z

Just wanted to say thanks for making this and I'll look into this soon

Murderlon

Wouldn't it make more sense to store the expiration instead of the creation date in the file model? Then we don't do creation.getTime() + this.getExpiration() * 60_000 every time and we can remove the getExperiration method?

lib/stores/FileStore.ts

lib/handlers/PostHandler.ts

Murderlon · 2022-10-31T19:37:22Z

We would also need a couple of tests for this

…actors Changed server expiration time options to use milliseconds instead of miuntes. Using await inside async function. And refactored expiration's implementation inside PostHandler

…pired

bypie5 · 2022-11-02T19:52:10Z

Some End-to-end tests have been added to test this feature

mitjap · 2022-11-02T20:21:43Z

Should PATCH, HEAD and possibly other requests check this expiration date and return 404 even if the cleanup function has not (yet) been called? This is especially the case when automatic external object removal happens (https://docs.aws.amazon.com/AmazonS3/latest/userguide/mpu-abort-incomplete-mpu-lifecycle-config.html) and user never does manual cleanup.

I can also think of various race conditions that can happen when cleanup occurs while user is still uploading a file.
Should active uploads be aborted when expiration time is reached?

Try this (extreme and unlikely) scenario. Run deleteExpired every second or so. Set expiration to one second. Upload file that will take at least 10 second, possibly in multiple chunks. 🤔 Expected behavior is that upload will be removed in between chunks and next chunk will return error 404. But in reality I guess there will (probably) be some kind of server error.

This can cause weird and hard to track errors. Especially with other stores that might follow similar pattern when implementing.

mitjap · 2022-11-02T20:32:11Z

Is it possible to make expiration extension datastore agnostic? I think every datastore can implement expiration extension (can share same deleteExpired code) given that it implements termination extension (has remove method`) and has enumerable configstore.

lib/stores/FileStore.ts

mitjap · 2022-11-02T21:21:57Z

lib/stores/FileStore.ts

+    const fileStats = await Promise.all(
+      Object.keys(this.configstore.all).map((file_id) => {
+        return this.getUpload(file_id)
+      })
+    )
+
+    let index = 0
+    for (const file_id of Object.keys(this.configstore.all)) {
+      try {
+        const stats = fileStats[index]
+        const info = this.configstore.get(file_id)


You call this.configstore.all two times and every time it returns all data which you discard and then later call this.configstore.get to retrieve discarded data. Note that everytime you read from or write to configstore (https://www.npmjs.com/package/configstore) entire file is read and parsed from disk or serialized and written to disk.

Given that N files are stored in configstore, this function does 2 + 3 * N (sync) reads and 2* N (sync) writes. I think this can be optimized to 1 read (if you leave this.configstore.all as is - see other comment) and N writes.

I'm afraid this function will stall server when performing this function, especially when there are lots of files on server. Can you try to run and time this function when there are 1k, 10k and 100k (can be small) files on server? If you time some HEAD requests while this task is in progress would be perfect 😜

Thanks for pointing out that the whole file is being read on configstore accesses! I refactored that portion of the code to only read from config store once like you suggested.

After that, I did some timing of HEAD requests of a server with 1k and 10k files. I timed the requests before the files were deleted, while the files were being deleted, and after all files had been deleted. Here are the summarized results.

In both cases you can see a small differences in time between HEAD requests made with a non-zero number of uploads saved in the configstore. Additionally there is a much larger difference in time between HEAD requests while the server is deleting and when it is not.

1000 files: max stall time 845ms || max stall time before deleting 33ms || max stall time after deleting 10ms

(now: Sun Nov 13 2022 16:17:15 GMT-0800 (Pacific Standard Time)) head request took [18ms] size on disk: 1 (now: Sun Nov 13 2022 16:17:15 GMT-0800 (Pacific Standard Time)) head request took [17ms] size on disk: 1 (now: Sun Nov 13 2022 16:17:15 GMT-0800 (Pacific Standard Time)) head request took [18ms] size on disk: 1 (now: Sun Nov 13 2022 16:17:17 GMT-0800 (Pacific Standard Time)) head request took [16ms] size on disk: 1 (now: Sun Nov 13 2022 16:17:17 GMT-0800 (Pacific Standard Time)) head request took [15ms] size on disk: 1 (now: Sun Nov 13 2022 16:17:24 GMT-0800 (Pacific Standard Time)) head request took [19ms] size on disk: 1 (now: Sun Nov 13 2022 16:17:24 GMT-0800 (Pacific Standard Time)) head request took [18ms] size on disk: 1 (now: Sun Nov 13 2022 16:17:24 GMT-0800 (Pacific Standard Time)) head request took [19ms] size on disk: 1 (now: Sun Nov 13 2022 16:17:26 GMT-0800 (Pacific Standard Time)) head request took [31ms] size on disk: 1 (now: Sun Nov 13 2022 16:17:26 GMT-0800 (Pacific Standard Time)) head request took [32ms] size on disk: 1 (now: Sun Nov 13 2022 16:17:26 GMT-0800 (Pacific Standard Time)) head request took [33ms] size on disk: 1 (now: Sun Nov 13 2022 16:17:27 GMT-0800 (Pacific Standard Time)) head request took [20ms] size on disk: 1 (now: Sun Nov 13 2022 16:17:27 GMT-0800 (Pacific Standard Time)) head request took [19ms] size on disk: 1 (now: Sun Nov 13 2022 16:17:27 GMT-0800 (Pacific Standard Time)) head request took [18ms] size on disk: 1 // SERVER HAS ABOUT 1k uploads // STARTED DELETING 1000 expired uploads (now: Sun Nov 13 2022 16:17:29 GMT-0800 (Pacific Standard Time)) head request took [523ms] size on disk: 1 (now: Sun Nov 13 2022 16:17:29 GMT-0800 (Pacific Standard Time)) head request took [522ms] size on disk: 1 (now: Sun Nov 13 2022 16:17:29 GMT-0800 (Pacific Standard Time)) head request took [523ms] size on disk: 1 (now: Sun Nov 13 2022 16:17:29 GMT-0800 (Pacific Standard Time)) head request took [522ms] size on disk: 1 (now: Sun Nov 13 2022 16:17:31 GMT-0800 (Pacific Standard Time)) head request took [1255ms] size on disk: 1 (now: Sun Nov 13 2022 16:17:31 GMT-0800 (Pacific Standard Time)) head request took [1254ms] size on disk: 1 (now: Sun Nov 13 2022 16:17:31 GMT-0800 (Pacific Standard Time)) head request took [1253ms] size on disk: 1 (now: Sun Nov 13 2022 16:17:31 GMT-0800 (Pacific Standard Time)) head request took [1254ms] size on disk: 1 HEAD http://localhost:1080/files/73eac9cbb34523adcb603336870ad53e net::ERR_ABORTED 410 (Gone) (now: Sun Nov 13 2022 16:17:32 GMT-0800 (Pacific Standard Time)) head request took [908ms] size on disk: null HEAD http://localhost:1080/files/a9b1963cad5c1d6801d19b8bfe595730 net::ERR_ABORTED 410 (Gone) (now: Sun Nov 13 2022 16:17:32 GMT-0800 (Pacific Standard Time)) head request took [912ms] size on disk: null HEAD http://localhost:1080/files/3e28dfe79bef43cd16fa70b25415790f net::ERR_ABORTED 410 (Gone) (now: Sun Nov 13 2022 16:17:32 GMT-0800 (Pacific Standard Time)) head request took [912ms] size on disk: null HEAD http://localhost:1080/files/2b47e539fd2d0957e1f607e897a8d814 net::ERR_ABORTED 410 (Gone) (now: Sun Nov 13 2022 16:17:32 GMT-0800 (Pacific Standard Time)) head request took [912ms] size on disk: null HEAD http://localhost:1080/files/3b939a30c15e99c7e35c2d7e9e27599e net::ERR_ABORTED 410 (Gone) (now: Sun Nov 13 2022 16:17:32 GMT-0800 (Pacific Standard Time)) head request took [912ms] size on disk: null HEAD http://localhost:1080/files/fe424f6f0859b2ab31d07f92d6ef2339 net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:33 GMT-0800 (Pacific Standard Time)) head request took [280ms] size on disk: null HEAD http://localhost:1080/files/27f8cab99b364753e7e1580f07f6893f net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:33 GMT-0800 (Pacific Standard Time)) head request took [290ms] size on disk: null HEAD http://localhost:1080/files/33acc6cdad8e7dafb36efadf3b7b2011 net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:33 GMT-0800 (Pacific Standard Time)) head request took [293ms] size on disk: null HEAD http://localhost:1080/files/29dc9c4361e569f167a654bcb512ef55 net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:33 GMT-0800 (Pacific Standard Time)) head request took [295ms] size on disk: null HEAD http://localhost:1080/files/5579eec897d94550ddff092c61efc170 net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:33 GMT-0800 (Pacific Standard Time)) head request took [296ms] size on disk: null HEAD http://localhost:1080/files/e21dcb5f543308f3bcf515698b6944e7 net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:35 GMT-0800 (Pacific Standard Time)) head request took [755ms] size on disk: null HEAD http://localhost:1080/files/e3468d2ef2be002dfecdfe1f41f2c67a net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:35 GMT-0800 (Pacific Standard Time)) head request took [754ms] size on disk: null HEAD http://localhost:1080/files/5df07d1acc516bee909b7bb084455250 net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:35 GMT-0800 (Pacific Standard Time)) head request took [755ms] size on disk: null HEAD http://localhost:1080/files/d9e8495da7f037be255a885164686f09 net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:35 GMT-0800 (Pacific Standard Time)) head request took [756ms] size on disk: null HEAD http://localhost:1080/files/3d0a16f0d9ded9811852b910f4f81a95 net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:35 GMT-0800 (Pacific Standard Time)) head request took [757ms] size on disk: null HEAD http://localhost:1080/files/2a8c3675b6b8ec6ad116aad30f0ca4aa 404 (Not Found) HEAD http://localhost:1080/files/f8ef5564d5f482713d34ba6cee9f614f net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:38 GMT-0800 (Pacific Standard Time)) head request took [842ms] size on disk: null HEAD http://localhost:1080/files/1c77fce6374dce8f2f89be1ed83086da net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:38 GMT-0800 (Pacific Standard Time)) head request took [842ms] size on disk: null HEAD http://localhost:1080/files/adb586616bbc3e24b78fa4372ee1a217 net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:38 GMT-0800 (Pacific Standard Time)) head request took [844ms] size on disk: null HEAD http://localhost:1080/files/c791175fd34635b3ab99396a23fed31f net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:38 GMT-0800 (Pacific Standard Time)) head request took [845ms] size on disk: null HEAD http://localhost:1080/files/29100a97f095419c284d4ed3ef43aacc net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:38 GMT-0800 (Pacific Standard Time)) head request took [845ms] size on disk: null HEAD http://localhost:1080/files/f0e86f2684504588a4733e21ddd0cf65 net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:39 GMT-0800 (Pacific Standard Time)) head request took [296ms] size on disk: null HEAD http://localhost:1080/files/8c5ddb5a5de4c387a7fc4a941af7ffb2 net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:39 GMT-0800 (Pacific Standard Time)) head request took [296ms] size on disk: null HEAD http://localhost:1080/files/ad869753bd81c9d66ce267aaed7f3fcc net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:39 GMT-0800 (Pacific Standard Time)) head request took [296ms] size on disk: null HEAD http://localhost:1080/files/74d29c593573647f45c6f526a97553b0 net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:39 GMT-0800 (Pacific Standard Time)) head request took [297ms] size on disk: null HEAD http://localhost:1080/files/073b13c262cf04dd8ec4e5acc6764e45 net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:39 GMT-0800 (Pacific Standard Time)) head request took [297ms] size on disk: null HEAD http://localhost:1080/files/8d7af3c2249aba9fea52e3c833be349c 404 (Not Found) (now: Sun Nov 13 2022 16:17:42 GMT-0800 (Pacific Standard Time)) head request took [75ms] size on disk: null HEAD http://localhost:1080/files/e4ab5298539a2791d85514b42fbb4c92 404 (Not Found) (now: Sun Nov 13 2022 16:17:44 GMT-0800 (Pacific Standard Time)) head request took [534ms] size on disk: null HEAD http://localhost:1080/files/8ddf3eb41f998bd45f63122929ba78ce net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:44 GMT-0800 (Pacific Standard Time)) head request took [534ms] size on disk: null HEAD http://localhost:1080/files/472a804663ac224f044282d6189924d4 net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:44 GMT-0800 (Pacific Standard Time)) head request took [535ms] size on disk: null HEAD http://localhost:1080/files/18053a1d1fd92426cc83631736d1fb37 net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:45 GMT-0800 (Pacific Standard Time)) head request took [344ms] size on disk: null HEAD http://localhost:1080/files/92039f3f680ac9799193702f86e744df 404 (Not Found) HEAD http://localhost:1080/files/eeff787ba3f7d14f808679fef6d451df net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:48 GMT-0800 (Pacific Standard Time)) head request took [191ms] size on disk: null (now: Sun Nov 13 2022 16:17:50 GMT-0800 (Pacific Standard Time)) head request took [197ms] size on disk: null HEAD http://localhost:1080/files/309dac4c73a7082ccb0a6a78b99ac7c8 net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:50 GMT-0800 (Pacific Standard Time)) head request took [195ms] size on disk: null HEAD http://localhost:1080/files/dcdae6b739f7dcf1d0ff9fd9273f7f5e net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:50 GMT-0800 (Pacific Standard Time)) head request took [195ms] size on disk: null HEAD http://localhost:1080/files/82f1c4e8124943821d3f17bb15c6c3e7 net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:50 GMT-0800 (Pacific Standard Time)) head request took [195ms] size on disk: null HEAD http://localhost:1080/files/d48111a4ea88341a666bfd50bf83c2e5 net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:50 GMT-0800 (Pacific Standard Time)) head request took [194ms] size on disk: null HEAD http://localhost:1080/files/471b01c7187512b0d08386f268b232b3 net::ERR_ABORTED 404 (Not Found) // DONE DELETING EXPIRED FILES (now: Sun Nov 13 2022 16:17:51 GMT-0800 (Pacific Standard Time)) head request took [10ms] size on disk: null HEAD http://localhost:1080/files/30fc791a606842f9e68637e53767e6d6 net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:51 GMT-0800 (Pacific Standard Time)) head request took [9ms] size on disk: null HEAD http://localhost:1080/files/40926a51be87a5b42c14d491f1ace424 net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:51 GMT-0800 (Pacific Standard Time)) head request took [9ms] size on disk: null HEAD http://localhost:1080/files/f341e54b43e0ef0f0585ad514f54d442 net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:51 GMT-0800 (Pacific Standard Time)) head request took [10ms] size on disk: null HEAD http://localhost:1080/files/1d2efc73772ff855a87853a3d1f9d97a net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:51 GMT-0800 (Pacific Standard Time)) head request took [9ms] size on disk: null HEAD http://localhost:1080/files/7bdfd9b697ea6308b2dd2c91d8332e3b net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:52 GMT-0800 (Pacific Standard Time)) head request took [9ms] size on disk: null HEAD http://localhost:1080/files/1881b009ef577f01daf4885d6c1d5ee6 net::ERR_ABORTED 404 (Not Found) (now: Sun Nov 13 2022 16:17:52 GMT-0800 (Pacific Standard Time)) head request took [8ms] size on disk: null HEAD http://localhost:1080/files/bf8b3b2503e005e56282ff6a1b0807fc net::ERR_ABORTED 404 (Not Found)

10000 files: max stall time 93492ms || max stall time before deleting 73ms || max stall time after deleting 13ms

[Sun Nov 13 2022 17:51:42 GMT-0800 (Pacific Standard Time)] Deleted 0 expired uploads [Sun Nov 13 2022 17:51:52 GMT-0800 (Pacific Standard Time)] Deleted 0 expired uploads [Sun Nov 13 2022 17:52:02 GMT-0800 (Pacific Standard Time)] Deleted 0 expired uploads [Sun Nov 13 2022 17:52:12 GMT-0800 (Pacific Standard Time)] Deleted 0 expired uploads [Sun Nov 13 2022 17:52:22 GMT-0800 (Pacific Standard Time)] Deleted 0 expired uploads [Sun Nov 13 2022 17:52:32 GMT-0800 (Pacific Standard Time)] Deleted 0 expired uploads -> (now: Sun Nov 13 2022 17:52:38 GMT-0800 (Pacific Standard Time)) head request took [73ms] [Sun Nov 13 2022 17:52:42 GMT-0800 (Pacific Standard Time)] Deleted 0 expired uploads -> (now: Sun Nov 13 2022 17:52:43 GMT-0800 (Pacific Standard Time)) head request took [68ms] [Sun Nov 13 2022 17:52:52 GMT-0800 (Pacific Standard Time)] Deleted 0 expired uploads // CONFIGSTORE HAS ~10_000 entries // SERVER STARTS DELETING EXPIRED UPLOADS EVERY 10 SECONDS [Sun Nov 13 2022 17:53:12 GMT-0800 (Pacific Standard Time)] Deleted 384 expired uploads [Sun Nov 13 2022 17:53:23 GMT-0800 (Pacific Standard Time)] Deleted 428 expired uploads [Sun Nov 13 2022 17:53:35 GMT-0800 (Pacific Standard Time)] Deleted 458 expired uploads -> (now: Sun Nov 13 2022 17:53:35 GMT-0800 (Pacific Standard Time)) head request took [28089ms] [Sun Nov 13 2022 17:53:48 GMT-0800 (Pacific Standard Time)] Deleted 522 expired uploads [Sun Nov 13 2022 17:54:02 GMT-0800 (Pacific Standard Time)] Deleted 601 expired uploads [Sun Nov 13 2022 17:54:18 GMT-0800 (Pacific Standard Time)] Deleted 662 expired uploads -> (now: Sun Nov 13 2022 17:54:18 GMT-0800 (Pacific Standard Time)) head request took [60740ms] [Sun Nov 13 2022 17:54:33 GMT-0800 (Pacific Standard Time)] Deleted 661 expired uploads [Sun Nov 13 2022 17:54:46 GMT-0800 (Pacific Standard Time)] Deleted 601 expired uploads [Sun Nov 13 2022 17:54:57 GMT-0800 (Pacific Standard Time)] Deleted 542 expired uploads [Sun Nov 13 2022 17:55:06 GMT-0800 (Pacific Standard Time)] Deleted 432 expired uploads -> (now: Sun Nov 13 2022 17:55:01 GMT-0800 (Pacific Standard Time)) head request took [93492ms] [Sun Nov 13 2022 17:55:16 GMT-0800 (Pacific Standard Time)] Deleted 361 expired uploads [Sun Nov 13 2022 17:55:24 GMT-0800 (Pacific Standard Time)] Deleted 336 expired uploads [Sun Nov 13 2022 17:55:33 GMT-0800 (Pacific Standard Time)] Deleted 317 expired uploads [Sun Nov 13 2022 17:55:43 GMT-0800 (Pacific Standard Time)] Deleted 311 expired uploads [Sun Nov 13 2022 17:55:54 GMT-0800 (Pacific Standard Time)] Deleted 308 expired uploads [Sun Nov 13 2022 17:56:04 GMT-0800 (Pacific Standard Time)] Deleted 293 expired uploads -> (now: Sun Nov 13 2022 17:56:04 GMT-0800 (Pacific Standard Time)) head request took [6050ms] [Sun Nov 13 2022 17:56:14 GMT-0800 (Pacific Standard Time)] Deleted 293 expired uploads [Sun Nov 13 2022 17:56:23 GMT-0800 (Pacific Standard Time)] Deleted 287 expired uploads [Sun Nov 13 2022 17:56:32 GMT-0800 (Pacific Standard Time)] Deleted 281 expired uploads [Sun Nov 13 2022 17:56:43 GMT-0800 (Pacific Standard Time)] Deleted 278 expired uploads [Sun Nov 13 2022 17:56:52 GMT-0800 (Pacific Standard Time)] Deleted 274 expired uploads [Sun Nov 13 2022 17:57:02 GMT-0800 (Pacific Standard Time)] Deleted 261 expired uploads -> (now: Sun Nov 13 2022 17:57:02 GMT-0800 (Pacific Standard Time)) head request took [1483ms] [Sun Nov 13 2022 17:57:12 GMT-0800 (Pacific Standard Time)] Deleted 261 expired uploads [Sun Nov 13 2022 17:57:21 GMT-0800 (Pacific Standard Time)] Deleted 260 expired uploads [Sun Nov 13 2022 17:57:32 GMT-0800 (Pacific Standard Time)] Deleted 262 expired uploads -> (now: Sun Nov 13 2022 17:57:37 GMT-0800 (Pacific Standard Time)) head request took [4ms] [Sun Nov 13 2022 17:57:41 GMT-0800 (Pacific Standard Time)] Deleted 251 expired uploads [Sun Nov 13 2022 17:57:48 GMT-0800 (Pacific Standard Time)] Deleted 75 expired uploads // SERVER DONE DELETING // CONFIGSTORE HAS 0 ENTRIES [Sun Nov 13 2022 17:57:57 GMT-0800 (Pacific Standard Time)] Deleted 0 expired uploads [Sun Nov 13 2022 17:58:07 GMT-0800 (Pacific Standard Time)] Deleted 0 expired uploads [Sun Nov 13 2022 17:58:17 GMT-0800 (Pacific Standard Time)] Deleted 0 expired uploads

Raw text data from debug script console output:
1k.txt
10k.txt

lib/stores/FileStore.ts

lib/models/File.ts

lib/stores/FileStore.ts

bypie5 · 2022-11-05T03:02:27Z

@mitjap To address your point:

Should PATCH, HEAD and possibly other requests check this expiration date and return 404 even if the cleanup function has not (yet) been called?

I think we might want to return 410 in those cases instead of 404. My reasoning is because of the following language in the specs:

If a Client does attempt to resume an upload which has since been removed by the Server, the Server SHOULD respond with the404 Not Found or 410 Gone status. The latter one SHOULD be used if the Server is keeping track of expired uploads. In both cases the Client SHOULD start a new upload. - Tus specs

But that is a good point you brought up about cleaning up an upload while someone is still trying to send data.

Should active uploads be aborted when expiration time is reached?

I think it would make sense to have expired uploads be aborted and report some error (410) back to the client. I will go ahead and try your example too and report back some observations.

Follow up from running example test case:

So, I ran a test where I created an upload that expired after one second and invoked deleteExpired in a loop very quickly while uploading a file with multiple PATCH requests that would take 10 seconds to finish. Without returning a response for expired uploads, the client can either see a 404 error code or 500 (like mitjap mentioned). The 500 error was happening since the server was still trying to write the an upload that had its associated data cleaned up.

However, we can prevent the 500 error from happening by returning 410 when a file is detected to be expired. This caused my test to either give the client a 404 error code or a 410 error code. In this case, the PATCH request handler would return with a defined error before trying to write to a file that could potentially be deleted from underneath its feet.

Added tests and removed a test that was to specific to timing

Acconut · 2022-11-05T17:51:16Z

Wow, that is an amazing PR! As a general question, we should decide if the upload expiration should be counted since the creation time or the last modification time. The spec allows both and this PR implements the former. Just to double check that this is the way we want to implement it right now.

…return string[] instead of Record<string, File> Apart of addressing comments on PR

Murderlon · 2022-11-09T20:38:56Z

Thanks for applying the feedback and writing tests. This is starting to look really good. There are currently some merge conflicts because I renamed the File model to Upload and changed the names in there. The types should be a lot better but unfortunately there are now some conflicts.

lib/models/Upload.ts

Murderlon · 2022-11-16T19:00:52Z

Bit confusing why the tests fail on Node 19.x but not on the others 🤔

…/tus-node-server into feat-expiration-filestore

Link to comments on PR: tus#320 (comment) and tus#320 (comment)

bypie5 · 2022-11-17T05:03:54Z

Bit confusing why the tests fail on Node 19.x but not on the others 🤔

I am also confused as well... at least for the failing PATCH test requests, it looks they are all erroneously detecting req.headers['upload-offset'] as undefined when running the test with node 19:

At the moment, I am not sure why this would be the case. Is this also happening on the 1.x branch too? Running the tests locally with node version 19 seems to suggest that these tests are also failing there too:

Murderlon · 2022-11-17T18:00:42Z

I ran the test locally on Node.js 19.0.1 and it ran successful, then upgraded to 19.1.0 and it failed...

They can't make a breaking change like this in a feature version I'd reckon so perhaps it's a Node bug?

Murderlon · 2022-11-17T18:40:41Z

Pinned the CI version to 19.0.1 for now.

Murderlon · 2022-11-17T21:18:11Z

I'll merge it tomorrow if no other reviews come in by then :)

Murderlon · 2022-11-18T15:09:53Z

Thanks a lot for this big contribution!

* 1.x: 0.9.0 Allow `credentials` instead of key/secret for the S3 store (#282) Update engines.node version requirement Add the `Expiration` extension, implement it in `FileStore` (#320) Lock CI Node.js version to 19.0.1 Clarify example for Fastify (#311)

piranna and others added 2 commits October 10, 2022 11:31

Added implementation of expiration extension to FileStore

8cefdce

bypie5 added 2 commits October 25, 2022 20:50

Merge branch '1.x' into feat-expiration-filestore

b4b10da

Merged in recent changes to 1.x

60a5935

Murderlon reviewed Oct 31, 2022

View reviewed changes

lib/stores/FileStore.ts Outdated Show resolved Hide resolved

lib/stores/FileStore.ts Outdated Show resolved Hide resolved

lib/handlers/PostHandler.ts Outdated Show resolved Hide resolved

Murderlon requested a review from Acconut October 31, 2022 19:37

bypie5 added 2 commits November 1, 2022 19:32

Added tests for FileStore expiration extension and addressed some ref…

a8f23cc

…actors Changed server expiration time options to use milliseconds instead of miuntes. Using await inside async function. And refactored expiration's implementation inside PostHandler

Refactored away callback in deleteExpired and added test for deleteEx…

1c9dab5

…pired

mitjap reviewed Nov 2, 2022

View reviewed changes

lib/stores/FileStore.ts Outdated Show resolved Hide resolved

mitjap reviewed Nov 2, 2022

View reviewed changes

HEAD and PATCH respond with 410 for expired uploads

da7edd7

Added tests and removed a test that was to specific to timing

Made expirationPeriodInMilliseconds default value be 0 and Store.all …

cc8d8fe

…return string[] instead of Record<string, File> Apart of addressing comments on PR

bypie5 added 2 commits November 12, 2022 20:54

Merge branch '1.X' into feat-expiration-filestore

9281dd1

Reduced number of reads of configstore to 1 in deleteExpired

8bdc05a

Murderlon reviewed Nov 16, 2022

View reviewed changes

lib/models/Upload.ts Outdated Show resolved Hide resolved

lib/models/Upload.ts Outdated Show resolved Hide resolved

bypie5 force-pushed the feat-expiration-filestore branch from 5c6838c to d89ed78 Compare November 17, 2022 02:50

bypie5 added 3 commits November 16, 2022 18:54

Merge branch 'feat-expiration-filestore' of https://github.com/bypie5…

e92a612

…/tus-node-server into feat-expiration-filestore

Applied suggestions from Pull Request for Upload type.

31f0409

Link to comments on PR: tus#320 (comment) and tus#320 (comment)

Fixed conflict in README

89f6888

Merge branch '1.x' into feat-expiration-filestore

2e465b8

aduh95 mentioned this pull request Nov 17, 2022

http: headers(Distinct), trailers(Distinct) setters to be no-op nodejs/node#45176

Merged

Murderlon approved these changes Nov 17, 2022

View reviewed changes

Murderlon changed the title ~~Added implementation of expiration extension to FileStore~~ Add the Expiration extension, implement it in FileStore Nov 18, 2022

Murderlon merged commit e1f966c into tus:1.x Nov 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the `Expiration` extension, implement it in `FileStore` #320

Add the `Expiration` extension, implement it in `FileStore` #320

bypie5 commented Oct 26, 2022

bypie5 commented Oct 26, 2022 •

edited

Loading

Murderlon commented Oct 31, 2022

Murderlon left a comment

Murderlon commented Oct 31, 2022

bypie5 commented Nov 2, 2022

mitjap commented Nov 2, 2022 •

edited

Loading

mitjap commented Nov 2, 2022

mitjap Nov 2, 2022 •

edited

Loading

bypie5 Nov 14, 2022

bypie5 commented Nov 5, 2022 •

edited

Loading

Acconut commented Nov 5, 2022

Murderlon commented Nov 9, 2022

Murderlon commented Nov 16, 2022

bypie5 commented Nov 17, 2022 •

edited

Loading

Murderlon commented Nov 17, 2022

Murderlon commented Nov 17, 2022

Murderlon commented Nov 17, 2022

Murderlon commented Nov 18, 2022

Add the Expiration extension, implement it in FileStore #320

Add the Expiration extension, implement it in FileStore #320

Conversation

bypie5 commented Oct 26, 2022

bypie5 commented Oct 26, 2022 • edited Loading

Murderlon commented Oct 31, 2022

Murderlon left a comment

Choose a reason for hiding this comment

Murderlon commented Oct 31, 2022

bypie5 commented Nov 2, 2022

mitjap commented Nov 2, 2022 • edited Loading

mitjap commented Nov 2, 2022

mitjap Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

bypie5 Nov 14, 2022

Choose a reason for hiding this comment

bypie5 commented Nov 5, 2022 • edited Loading

Acconut commented Nov 5, 2022

Murderlon commented Nov 9, 2022

Murderlon commented Nov 16, 2022

bypie5 commented Nov 17, 2022 • edited Loading

Murderlon commented Nov 17, 2022

Murderlon commented Nov 17, 2022

Murderlon commented Nov 17, 2022

Murderlon commented Nov 18, 2022

Add the `Expiration` extension, implement it in `FileStore` #320

Add the `Expiration` extension, implement it in `FileStore` #320

bypie5 commented Oct 26, 2022 •

edited

Loading

mitjap commented Nov 2, 2022 •

edited

Loading

mitjap Nov 2, 2022 •

edited

Loading

bypie5 commented Nov 5, 2022 •

edited

Loading

bypie5 commented Nov 17, 2022 •

edited

Loading