Handle kill signal #123
Handle kill signal #123
Conversation
In order for the program to close somewhat cleanly, it now catches the signal and deletes the temporary file and folder before quitting. Also the temporary files have been renamed to have meaningful file names
This car is cute and all but on an operational standpoint it's not practical and even on my own computer with fonts that handle these kinds of characters, the car doesn't display in htop.
package.json
Outdated
| @@ -8,7 +8,6 @@ | |||
| "/dist" | |||
| ], | |||
| "bin": { | |||
| "🚘": "./dist/cjs/cli/cli.js", | |||
There was a problem hiding this comment.
Any reason to remove this bin emoji entry?
There was a problem hiding this comment.
As stated in the commit changing this,
The car is cute and fun but on an operational standpoint it is not usable.
In tty or on terminals not supporting this, it is hard to spot the process and to just enter the process name manually isn't possible this way.
I don't even see the car in htop even though my terminal supports the character which means that it is even incompatible with some applications that may be used on a server
There was a problem hiding this comment.
There is still the option to use ipfs-car command, so this change is out of scope of this PR. You can raise an issue to have it removed and explain the reasoning.
There was a problem hiding this comment.
I've reverted this change
src/blockstore/fs.ts
Outdated
| super() | ||
| this.path = `${os.tmpdir()}/${(parseInt(String(Math.random() * 1e9), 10)).toString() + Date.now()}` | ||
| this.path = path |
There was a problem hiding this comment.
This will be a breaking change release for users bringing their own Blockstore on pack. I think we should do this change, but with retro compatibility. Let's default to the original value here if no path is provided?
There was a problem hiding this comment.
This is meant for the temporary directory which instead of being named with random numbers is named with something relevant to what's being packed (which helps users finding which one it is).
Also, it allows reconstructing the folder name to delete it on SIGKILL or SIGTERM
There was a problem hiding this comment.
I understand and think your solution is better. But, also want to keep it compatible with older versions.
What I mean here is we should fallback to the temporary directory being name randomly for people who don't provide a name.
this.path = path || `${os.tmpdir()}/${(parseInt(String(Math.random() * 1e9), 10)).toString() + Date.now()}`This way, users that are relying on ipfs-car and providing a Blockstore from their side will not have unexpected changes without changing their code accordingly. Also, path is only relevant for FsBlockstore, so should really be optional and not required to keep interface the same for all blockstores
There was a problem hiding this comment.
I applied a change that should prevent custom blockstores from breaking, and still allow the clean up after a kill.
| @@ -35,7 +36,7 @@ export async function packToFs ({ input, output, blockstore: userBlockstore, has | |||
| // Move to work dir | |||
| if (!output) { | |||
| const basename = typeof input === 'string' ? path.parse(path.basename(input)).name : root.toString() | |||
| const filename = `${basename}.car` | |||
| const filename = basename === "/" ? "file.car" : `${basename}.car` | |||
There was a problem hiding this comment.
file.car will be problematic for multiple packs in parallel as they will use same filename. Can we generate a random name like we used to do in blockstore ?
There was a problem hiding this comment.
file.car is meant to be used when someone wants to pack up to his root (so someone wouldn't be packing its entire root at the same time multiple times).
But otherwise it was meant to not use ..car when . is passed as the input directory but resolve the name of this directory instead.
There was a problem hiding this comment.
What's the state of this discussion?
I believe that it shouldn't be an issue but LMK.
|
@Breigner01 can you also have a look into tests failing? TS compile is failing from what I can see |
Yeah I didn't see that, I might need to change the tests base on the changes I've made |
This reverts commit 709fd61.
This allows to not break custom blockstores and allow at the same time clean kill signal handling.
|
Thanks for the updates @Breigner01 Can you look into the tests too? |
I'll look into it on Monday, I can't see it clearly on mobile but have you reran the tests after the modifications I did recently? |
Thanks! Yeah, I ran CI for latest commit |
The temporary car file has been moved to the directory containing the folder or file being packed into the car.
This allows to not have to do multiple copies off of multiple drives (when the tmp folder may be on another drive).
The temporary file and folder have been renamed to make sense and allow people to see if the file is there and the name should make sense compared to the final one.
At the same time, now the kill signal is being handled and on kill, these are being deleted.