Skip to content
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

Crashing the osjs when uploading a file in osjs mountpoint #79

Closed
maryam4s26 opened this issue Jan 1, 2024 · 19 comments · Fixed by #80
Closed

Crashing the osjs when uploading a file in osjs mountpoint #79

maryam4s26 opened this issue Jan 1, 2024 · 19 comments · Fixed by #80
Labels
bug Something isn't working

Comments

@maryam4s26
Copy link

hi, @andersevenrud
According to the os-js/OS.js#840 and #77, it seems that the error has been resolved.
Unfortunately, the error has not been resolved.

@andersevenrud

This comment was marked as outdated.

@maryam4s26
Copy link
Author

maryam4s26 commented Jan 2, 2024

For me, crash can simply be happened by uploading a file in osjs mountpoint.
node version: 16.0.0
osjs-server version: 3.4.0

Screenshot from 2023-12-25 16-21-27

error is :

Error: ENOENT: no such file or directory, open '/tmp/upload_727a4fa83a184e7c5fa9774d6349f0c2'
Emitted 'error' event on ReadStream instance at:

I think the error that happened should be solved in this way #78

@andersevenrud

@andersevenrud

This comment was marked as resolved.

andersevenrud added a commit that referenced this issue Jan 2, 2024
Defers the VFS cleanup process and slightly cleans up the error
handling in the request chain to prevent crashing when streams
are destroyed by the runtime.

Fixes #79
@andersevenrud

This comment was marked as outdated.

@andersevenrud

This comment was marked as resolved.

andersevenrud added a commit that referenced this issue Jan 2, 2024
Defers the creation of streams in the VFS process to avoid
any cleanups that ends up in exceptions because they are
handled outside the scope/context.

Fixes #79
@andersevenrud
Copy link
Member

The correct fix for this is to refactor the upload stuff

I went ahead and did a minor refactor here, and instead of deferring the cleanup the creation of the stream is deferred.

This way there is no chance a Stream is created if any exception occurs in the VFS request pre-checks.

Should be good enough for now. The PR has been updated.

@andersevenrud
Copy link
Member

I actually think this might also have fixed an edge-case where temporary files never actually was cleaned up. I was messing around with one of my servers to try and reproduce this issue (which I actually did not manage... only on my Macbook at home) and saw there was quite a few old files that is probably from errors.

@andersevenrud andersevenrud added the bug Something isn't working label Jan 2, 2024
@andersevenrud

This comment was marked as duplicate.

@maryam4s26
Copy link
Author

I think the error that happened should be solved in this way #78

Just to comment on why I did not choose this. This solution prevents the crash from occurring, however it does not actually fix the reason for the crash, but rather "hides" the error from the system.

This means that any actual error with the stream (anything not related to the problem reported in this issue) would cause the requests to hang/fail. No error events ends with no exceptions, and no exceptions ends with no response :)

Thank you for your complete explanation.

@andersevenrud
Copy link
Member

I have tested this patch for 30 minutes on a couple of different machines and it seems to work fine to me.

Now I really hope it also fixes it for you, and can't wait to hear your results from trying out the PR 🤞

Funny thing is that this bug is probably 4+ years old, so thank you very much for reporting 😅

andersevenrud added a commit that referenced this issue Jan 3, 2024
Defers the creation of streams in the VFS process to avoid
any cleanups that ends up in exceptions because they are
handled outside the scope/context.

Fixes #79
@andersevenrud
Copy link
Member

I went ahead and merged the PR and published a new 3.4.1 version. From my testing this seems to work.

@maryam4s26
Copy link
Author

maryam4s26 commented Jan 3, 2024

Hello @andersevenrud .
Thanks for #80. I apologize for the delay, I have been testing #80.
I tested this for osjs And it didn't crash anymore.

But according to this, I have my own adapter for vfs, and if an error occurs when using the writefile request, I will get the same error again and the system will crash.
This also happened when I changed my node version to 16 and it didn't improve when I updated the server version to 3.4.1.

image

@andersevenrud
Copy link
Member

But according to this, I have my own adapter for vfs, and if an error occurs when using the writefile request, I will get the same error again and the system will crash.

Yeah, sadly my PR does not solve this issue when an error occurs further down the line in this context. I started on a new patch to refactor parts and use a new version of formidable, but don't really have the time to finish it until this weekend.

I'll send you a notification :)

@maryam4s26
Copy link
Author

Thank you very much.

@andersevenrud
Copy link
Member

I just got an idea for a possible simpler solution. Could you test this PR?

#81

@maryam4s26
Copy link
Author

Finallllllllllllly, it was fixed.
Thank you very much.

@andersevenrud
Copy link
Member

Sweet! I published a new version.

@maryam4s26
Copy link
Author

Why do you only increment the version in osjs-server?
Why don't you make any changes in osjs?

@andersevenrud
Copy link
Member

I don't really feel that it's needed / I'm lazy, hehe. The installation instructions contains a step for updating packages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants