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

Fix for #660 #661

Merged
merged 1 commit into from Mar 2, 2017

Conversation

Projects
None yet
3 participants
@Stevie-O
Contributor

Stevie-O commented Mar 1, 2017

This fixes #660

Don't emit the script destroyed signal before script is actually dest…
…royed

The script unloading code originally worked like this:

1. Destroy package
2. Emit 'script destroyed' signal
3. Unhook script's signal handlers

If a script added a 'script destroyed' signal handler, unloading
that script would cause the 'script destroyed' signal to be sent to the
(already destroyed) package.  This would cause a script error, which would
trigger a script unload, which would start the whole process over again,
until we run out of heap or stack space and segfault.

This commit simply reorders the operations so that the 'script destroyed'
signal is sent *after* the script is fully destroyed.
@LemonBoy

This comment has been minimized.

Member

LemonBoy commented Mar 1, 2017

It makes sense to do so because the script destroyed signal is only used internally and it doesn't call into the Perl side. The only caveat is that the script won't see the signal at all (do we care about this?)

@Stevie-O

This comment has been minimized.

Contributor

Stevie-O commented Mar 2, 2017

@LemonBoy I'm not sure what you mean when you say that it is 'only used internally and doesn't call into the Perl side', but you are right that the script itself can't get the script destroyed signal; it never could! By the time the perl_script_destroy function is called, pretty much everything the script needs in order to run has already been removed by perl_script_destroy_package.

I considered the idea of allowing a script to receive its own destroy signal, but rejected it for the following reasons:

  1. Such a thing would make it impossible for a user to unload a script without executing any code on it.
  2. If a script wants a to have a 'safe unload' mechanism, the script may register a custom command that cleans up and then unloads itself; if the user wants to do a clean unload, they can just run that command instead. (That should work fine; I think I'll test that next.)
  3. If the 'script destroyed' handler causes a script error, we're right back where we started.

All in all, it does not seem like a good idea to try to automatically notify scripts that they're being unloaded.

@ailin-nemui ailin-nemui merged commit 647ef19 into irssi:master Mar 2, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Mar 2, 2017

in general I would not expect scripts to do such mischievous things as registering this signal. It would be nice to have a safe scripting api but we do not at the moment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment