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

Do not close in finalizer on sbcl #29

Closed
wants to merge 1 commit into from
Closed

Conversation

ralt
Copy link

@ralt ralt commented May 6, 2020

SBCL itself is already doing the exact same thing, see:

https://github.com/sbcl/sbcl/blob/master/contrib/sb-simple-streams/internal.lisp#L656

which calls: https://github.com/sbcl/sbcl/blob/master/src/code/fd-stream.lisp#L2285-L2292

In my case, I ended up going down that rabbit hole because the FD reported by source (i.e. the underlying fd-stream), aka (sb-sys:fd-stream-fd (slot-value ironclad:*prng* 'ironclad::source)), had a different file descriptor in /proc/$pid/fd/. The behavior I was observing was a stuck read -- most likely because the real file descriptor stored in fd-stream was a socket.

I assume it is related to double-closing, one way or another.

SBCL itself is already doing the exact same thing, see:

https://github.com/sbcl/sbcl/blob/master/contrib/sb-simple-streams/internal.lisp#L656

which calls: https://github.com/sbcl/sbcl/blob/master/src/code/fd-stream.lisp#L2285-L2292

In my case, I ended up going down that rabbit hole because the FD reported by `source` (i.e. the underlying fd-stream) had a different file descriptor in `/proc/$pid/fd/`.

I assume it is related to double-closing, one way or another.
@ralt
Copy link
Author

ralt commented May 6, 2020

So, the issue with the different file descriptor is actually because it's a dumped image. The finalizer is fine on sbcl, but sbcl is still already doing it. So, do what you want with this PR :)

@ralt
Copy link
Author

ralt commented May 6, 2020

That said, maybe ironclad could only initialize *prng* on first use to avoid issues with dumped/restored images. (Which was the case before 36f3685, and it's why it used to work before I upgraded to latest ironclad.)

@stassats
Copy link
Contributor

stassats commented May 6, 2020

Or better yet, handle it in sb-ext:*init-hooks*/sb-ext:*save-hooks*

@glv2
Copy link
Collaborator

glv2 commented May 7, 2020

The finalizer closing '/dev/urandom' was added because of a file descriptor leak with CCL (issue #26). The double close on the stream with SBCL should not be an issue, as according to the hyperspec "it is permissible to close an already closed stream".

I pushed a commit (35cbdcc) to open '/dev/urandom' on first use of an os-prng instead of doing it when creating it. It should fix the problem with the stuck read in dumped images.

A workaround for version 0.49 could be to create a new default prng in the entry point function of a dumped image (with (setf crypto:*prng* (crypto:make-prng :os))).

@ralt
Copy link
Author

ralt commented May 7, 2020

Thank you! The first use thing is not necessarily perfect though, as a dumped image can still possibly end up with broken FDs, but that probably takes care of the most common issues.

@ralt ralt closed this May 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants