New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: add separate-thread concurrency test #305
base: main
Are you sure you want to change the base?
Conversation
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.
self review
@@ -48,7 +48,6 @@ async function writeFile (file: string, contents: Uint8Array): Promise<void> { | |||
// attempts to write the same block by two different function calls | |||
return | |||
} |
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.
should add the newline back here.
@@ -37,7 +37,7 @@ async function writeFile (file: string, contents: Uint8Array): Promise<void> { | |||
try { | |||
await writeAtomic(file, contents) | |||
} catch (err: any) { | |||
if (err.code === 'EPERM' && err.syscall === 'rename') { | |||
if (err.syscall === 'rename' && ['ENOENT', 'EPERM'].includes(err.code)) { |
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.
FYI that on my mac, an ENOENT
Error was firing on the rename
call.
example:
blockstore-fs: Error putting block [Error: ENOENT: no such file or directory, rename '/var/folders/bl/_gl5_59s11v7qz5ysd6bfgb00000gn/T/test-0.8216405349064326/LY/.63718.93' -> '/var/folders/bl/_gl5_59s11v7qz5ysd6bfgb00000gn/T/test-0.8216405349064326/LY/BCIQPGZJ6QLZOFG3OP45NLMSJUWGJCO72QQKHLDTB6FXIB6BDSLRQYLY.data'] {
blockstore-fs: errno: -2,
blockstore-fs: code: 'ERR_PUT_FAILED',
blockstore-fs: syscall: 'rename',
blockstore-fs: path: '/var/folders/bl/_gl5_59s11v7qz5ysd6bfgb00000gn/T/test-0.8216405349064326/LY/.63718.93',
blockstore-fs: dest: '/var/folders/bl/_gl5_59s11v7qz5ysd6bfgb00000gn/T/test-0.8216405349064326/LY/BCIQPGZJ6QLZOFG3OP45NLMSJUWGJCO72QQKHLDTB6FXIB6BDSLRQYLY.data'
blockstore-fs: }
I think if we validate that the final file exists and is F_OK, W_OK as we were doing for EPERM
, this is safe.
I also thought about checking that error.dest
matches, but it seems like that is effectively what we're doing with fs.access
, so this should work
return true | ||
} catch (err) { | ||
// eslint-disable-next-line no-console | ||
console.error('Error opening blockstore', err) |
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.
so we can get some error information in testing environment without dealing with logger? i can add in a logger if desired.
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.
rethrow the error here. done.
@@ -37,7 +37,7 @@ async function writeFile (path: string, contents: Uint8Array): Promise<void> { | |||
try { | |||
await writeAtomic(path, contents) | |||
} catch (err: any) { | |||
if (err.code === 'EPERM' && err.syscall === 'rename') { | |||
if (err.syscall === 'rename' && ['ENOENT', 'EPERM'].includes(err.code)) { |
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.
see comment from same changes in blockstore-fs: #305 (comment)
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.
looks like we're also getting an EPERM on 'open' with fast-atomic-write on windows:
Error: EPERM: operation not permitted, open 'C:\Users\RUNNER~1\AppData\Local\Temp\test-0.9268640174546598\LY.6280.3'
https://github.com/ipfs/js-stores/actions/runs/8757918340/job/24047367236?pr=305#step:5:687
This is not failing on windows with steno: #285
Note that https://github.com/ipfs/js-stores/actions/runs/8757918340/job/24037687392?pr=305#step:5:645 is failing here for windows, but passing in #285. going to retry the job. new job at https://github.com/ipfs/js-stores/actions/runs/8757918340?pr=305 |
findings in #305 (comment).. seem's like fast-write-atomic steps on itself with temp files when ran in a separate concurrent thread. |
Basically spawned from #285 (comment)
This PR adds a test to both datastore-fs and blockstore-fs that validates puts are somewhat concurrent safe across multiple threads.