plugin: Add support for closing plugins #20461

Open
zacps opened this Issue May 22, 2017 · 31 comments

Comments

Projects
None yet
10 participants
@zacps

zacps commented May 22, 2017

Adding support for closing plugins would give a simple way to do hot code reloading in long running applications like web servers.

This would add another method to src/plugin/plugin.go and a new file src/plugin/dl_close.go that would handle closing the dlhandle and destroying the go symbol map.
It would also add the handle as an un-exported unsafe pointer to the go plugin struct.

The main issue is how to signal that a plugin has been closed without breaking API compatibility. Currently the plugin has a channel called loaded which is closed when the plugin is loaded. The simplest solution is to add another channel which is closed when the plugin is closed, however this doesn't seem very 'neat' to me.

@davecheney

This comment has been minimized.

Show comment
Hide comment
@davecheney

davecheney May 22, 2017

Contributor

I don't see how this could be implemented safely. Other Go code could have references to functions and symbols loaded from the plugin-would unloading the plugin make those dangling references, or delay the unloading of a plugin until all the references have been reclaimed? Neither sound workable.

Contributor

davecheney commented May 22, 2017

I don't see how this could be implemented safely. Other Go code could have references to functions and symbols loaded from the plugin-would unloading the plugin make those dangling references, or delay the unloading of a plugin until all the references have been reclaimed? Neither sound workable.

@zacps

This comment has been minimized.

Show comment
Hide comment
@zacps

zacps May 22, 2017

Would it be possible to make close fail until the reference count of all symbols in the plugin reached 1, so that the plugin itself was the only structure that held a reference?

Alternatively you could have a different function which checked whether references to the symbols existed and if there were none close the dlhandle, leaving the destruction of the plugin object and any other references up to the program. While this is far from ideal I think with adequate caution it could be a workable solution.

zacps commented May 22, 2017

Would it be possible to make close fail until the reference count of all symbols in the plugin reached 1, so that the plugin itself was the only structure that held a reference?

Alternatively you could have a different function which checked whether references to the symbols existed and if there were none close the dlhandle, leaving the destruction of the plugin object and any other references up to the program. While this is far from ideal I think with adequate caution it could be a workable solution.

@davecheney

This comment has been minimized.

Show comment
Hide comment
@davecheney

davecheney May 22, 2017

Contributor

Would it be possible to make close fail until the reference count of all symbols in the plugin reached 1, so that the plugin itself was the only structure that held a reference?

This is spinny and racy.

Alternatively you could have a different function which checked whether references to the symbols existed and if there were none close the dlhandle, leaving the destruction of the plugin object and any other references up to the program. While this is far from ideal I think with adequate caution it could be a workable solution.

This sounds like a finaliser, with the associated problems of finaliser non determinism.

Contributor

davecheney commented May 22, 2017

Would it be possible to make close fail until the reference count of all symbols in the plugin reached 1, so that the plugin itself was the only structure that held a reference?

This is spinny and racy.

Alternatively you could have a different function which checked whether references to the symbols existed and if there were none close the dlhandle, leaving the destruction of the plugin object and any other references up to the program. While this is far from ideal I think with adequate caution it could be a workable solution.

This sounds like a finaliser, with the associated problems of finaliser non determinism.

@zacps

This comment has been minimized.

Show comment
Hide comment
@zacps

zacps May 22, 2017

Could you be a bit more specific about the second point?

zacps commented May 22, 2017

Could you be a bit more specific about the second point?

@davecheney

This comment has been minimized.

Show comment
Hide comment
@davecheney

davecheney May 22, 2017

Contributor

Your proposal is to add a way to free resources associated with a plugin. If this is delegated to a finaliser it becomes non deterministic when this freeing occurs as finalisers are not guaranteed to run promptly, or indeed, at all.

Contributor

davecheney commented May 22, 2017

Your proposal is to add a way to free resources associated with a plugin. If this is delegated to a finaliser it becomes non deterministic when this freeing occurs as finalisers are not guaranteed to run promptly, or indeed, at all.

@zacps

This comment has been minimized.

Show comment
Hide comment
@zacps

zacps May 22, 2017

I'm not suggesting that it's run automatically by the GC, or in a defer (unfortunately).

I'm suggesting that if the needs to be unloaded that the developer will keep track of references to it's symbols then when they no longer need them destroy the references then explicitly call close.

In that situation they will either forget to call close, leaving the handle open, close successfully or attempt to close when there are still references which will fail. It could possibly fail with a panic rather than a plain error as attempting the close a resource that still has a reference is a pretty big bug.

I don't see what issues this approach has beyond reliance on the developer to not make mistakes. It does introduce a need to keep track of references which is far from perfect and not something I'd like to introduce to go. However I think that it's worth having this for those that want to use it and understand the additional complexity it introduces.

zacps commented May 22, 2017

I'm not suggesting that it's run automatically by the GC, or in a defer (unfortunately).

I'm suggesting that if the needs to be unloaded that the developer will keep track of references to it's symbols then when they no longer need them destroy the references then explicitly call close.

In that situation they will either forget to call close, leaving the handle open, close successfully or attempt to close when there are still references which will fail. It could possibly fail with a panic rather than a plain error as attempting the close a resource that still has a reference is a pretty big bug.

I don't see what issues this approach has beyond reliance on the developer to not make mistakes. It does introduce a need to keep track of references which is far from perfect and not something I'd like to introduce to go. However I think that it's worth having this for those that want to use it and understand the additional complexity it introduces.

@bradfitz bradfitz added this to the Unplanned milestone May 22, 2017

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor May 22, 2017

Contributor

What if the plugin started a goroutine?

Go as a language and a runtime environment tends to avoid providing facilities that are easy to get wrong when getting them wrong causes the program to crash or behave unpredictably. I don't see how to make this at all reliable, so to me it doesn't seem to be a good fit for Go.

Contributor

ianlancetaylor commented May 22, 2017

What if the plugin started a goroutine?

Go as a language and a runtime environment tends to avoid providing facilities that are easy to get wrong when getting them wrong causes the program to crash or behave unpredictably. I don't see how to make this at all reliable, so to me it doesn't seem to be a good fit for Go.

@zacps

This comment has been minimized.

Show comment
Hide comment
@zacps

zacps May 22, 2017

goroutines would have to be handled in the same way, requiring the developer to close them.

I can understand not wanting to introduce functionality that's easy to misuse. I just think that it's better to have the ability to do it if you know what you're doing. I guess it does go against the philosophy of go though.

zacps commented May 22, 2017

goroutines would have to be handled in the same way, requiring the developer to close them.

I can understand not wanting to introduce functionality that's easy to misuse. I just think that it's better to have the ability to do it if you know what you're doing. I guess it does go against the philosophy of go though.

@davecheney

This comment has been minimized.

Show comment
Hide comment
@davecheney

davecheney May 23, 2017

Contributor
Contributor

davecheney commented May 23, 2017

@zacps

This comment has been minimized.

Show comment
Hide comment
@zacps

zacps May 23, 2017

I took a quick look at runtime but couldn't find where exactly the goroutine is created. Are goroutines not closed when they return?

zacps commented May 23, 2017

I took a quick look at runtime but couldn't find where exactly the goroutine is created. Are goroutines not closed when they return?

@davecheney

This comment has been minimized.

Show comment
Hide comment
@davecheney

davecheney May 23, 2017

Contributor
Contributor

davecheney commented May 23, 2017

@zacps

This comment has been minimized.

Show comment
Hide comment
@zacps

zacps May 23, 2017

Ok, that's what I thought.

I personally think that manual bookkeeping wouldn't be too bad. If you're writing something designed to be loaded/unloaded you're probably going to make sure you keep track of references and goroutines as is.

I'm trying to think whether there's a better way to do this. I guess it's not something other languages have had problems with.

zacps commented May 23, 2017

Ok, that's what I thought.

I personally think that manual bookkeeping wouldn't be too bad. If you're writing something designed to be loaded/unloaded you're probably going to make sure you keep track of references and goroutines as is.

I'm trying to think whether there's a better way to do this. I guess it's not something other languages have had problems with.

@zacps

This comment has been minimized.

Show comment
Hide comment
@zacps

zacps May 24, 2017

I have another idea for how this could be solved, it's not simple though and I'm not sure whether it would make sense.

Basically I think it would be possible to implement a compile-time check that ensured there was no possibility for a dangling reference when a program was closed.

The compiler could check to see if close was called on any plugin object in the program. If it was we'd find all of the lookup calls associated with that plugin and determine whether there were any paths that could leave a live reference when or after closed was called.

I have no idea how you'd implement this or whether it'd be fast enough to be viable. Just an idea.

zacps commented May 24, 2017

I have another idea for how this could be solved, it's not simple though and I'm not sure whether it would make sense.

Basically I think it would be possible to implement a compile-time check that ensured there was no possibility for a dangling reference when a program was closed.

The compiler could check to see if close was called on any plugin object in the program. If it was we'd find all of the lookup calls associated with that plugin and determine whether there were any paths that could leave a live reference when or after closed was called.

I have no idea how you'd implement this or whether it'd be fast enough to be viable. Just an idea.

@DemiMarie

This comment has been minimized.

Show comment
Hide comment
@DemiMarie

DemiMarie Jun 4, 2017

What about having the GC close any plugins that are no longer referenced?

That's what the JVM does. And forcing a full GC whenever Go code must be unloaded is not hard.

DemiMarie commented Jun 4, 2017

What about having the GC close any plugins that are no longer referenced?

That's what the JVM does. And forcing a full GC whenever Go code must be unloaded is not hard.

@davecheney

This comment has been minimized.

Show comment
Hide comment
@davecheney

davecheney Jun 4, 2017

Contributor
Contributor

davecheney commented Jun 4, 2017

@zacps

This comment has been minimized.

Show comment
Hide comment
@zacps

zacps Jun 4, 2017

@davecheney Any comment on making it a compile-time check?

zacps commented Jun 4, 2017

@davecheney Any comment on making it a compile-time check?

@davecheney

This comment has been minimized.

Show comment
Hide comment
@davecheney

davecheney Jun 4, 2017

Contributor
Contributor

davecheney commented Jun 4, 2017

@zacps

This comment has been minimized.

Show comment
Hide comment
@zacps

zacps Jun 4, 2017

I don't see how it's like a finaliser, at least in the aspect of non-determinism.

It is pretty similar to rust's general approach; I don't think that's a bad thing though.

zacps commented Jun 4, 2017

I don't see how it's like a finaliser, at least in the aspect of non-determinism.

It is pretty similar to rust's general approach; I don't think that's a bad thing though.

@tarndt

This comment has been minimized.

Show comment
Hide comment
@tarndt

tarndt Jun 5, 2017

If I load a plugin foobar-v1.so that has foobar() I could wrap it in my own functionfoobarWapper() that increments/decrements a waitgroup on every call. Things that want to call foobar do so accessing foobarWraper atomically and calling it. Later on I load foobar-v2.so, wrap it in a new wrapper, and atomically swap it out for out for the old foobarWrapper. Now I can use the old foobarWraper's wait group to make sure no one is active and when that is the case call dlclose using cgo.

This all would depend on the user not creating goroutines, but that could be checked for at the source level if you are building plugins from user submitted code.

@zacps Maybe something like the above would work for you.

tarndt commented Jun 5, 2017

If I load a plugin foobar-v1.so that has foobar() I could wrap it in my own functionfoobarWapper() that increments/decrements a waitgroup on every call. Things that want to call foobar do so accessing foobarWraper atomically and calling it. Later on I load foobar-v2.so, wrap it in a new wrapper, and atomically swap it out for out for the old foobarWrapper. Now I can use the old foobarWraper's wait group to make sure no one is active and when that is the case call dlclose using cgo.

This all would depend on the user not creating goroutines, but that could be checked for at the source level if you are building plugins from user submitted code.

@zacps Maybe something like the above would work for you.

@zacps

This comment has been minimized.

Show comment
Hide comment
@zacps

zacps Jun 9, 2017

Wouldn't you have to either use code generation or lose type safety though?

zacps commented Jun 9, 2017

Wouldn't you have to either use code generation or lose type safety though?

@tarndt

This comment has been minimized.

Show comment
Hide comment
@tarndt

tarndt Jun 10, 2017

Wouldn't you have to either use code generation or lose type safety though?

@zacps I'm not sure how type safety is related to this...

This all would depend on the user not creating goroutines, but that could be checked for at the source level if you are building plugins from user submitted code.

So using my approach you would write a program to vet the user code before building the plugin. Probably by having a white-list of importable packages, and searching the code to get for invocations of "go X" and any functions in white listed packages you want to also blacklist.

tarndt commented Jun 10, 2017

Wouldn't you have to either use code generation or lose type safety though?

@zacps I'm not sure how type safety is related to this...

This all would depend on the user not creating goroutines, but that could be checked for at the source level if you are building plugins from user submitted code.

So using my approach you would write a program to vet the user code before building the plugin. Probably by having a white-list of importable packages, and searching the code to get for invocations of "go X" and any functions in white listed packages you want to also blacklist.

@DemiMarie

This comment has been minimized.

Show comment
Hide comment
@DemiMarie

DemiMarie Jun 10, 2017

@jaekwon

This comment has been minimized.

Show comment
Hide comment
@jaekwon

jaekwon Dec 16, 2017

I second starting with an unsafe way to load and unload plugins, with the expectation that the user will sanitize code before compiling it. It's OK if attempting to unload a plugin panics when there exist dangling pointers/references, and it's also OK if the panic happens later nondeterministically when the dangling pointers/references are accessed.

jaekwon commented Dec 16, 2017

I second starting with an unsafe way to load and unload plugins, with the expectation that the user will sanitize code before compiling it. It's OK if attempting to unload a plugin panics when there exist dangling pointers/references, and it's also OK if the panic happens later nondeterministically when the dangling pointers/references are accessed.

@Azareal

This comment has been minimized.

Show comment
Hide comment
@Azareal

Azareal Dec 16, 2017

I currently use code generation for templates. I have a template transpiler which transpiles text/template templates to Pure Go (it's a little coupled with my application right now, but I could always split it off and it currently misses a few features).

It would be nice, if I could reload these templates on the fly without restarting the entire instance, something impossible without the ability to close plugins or rolling my own JIT compiler.

Azareal commented Dec 16, 2017

I currently use code generation for templates. I have a template transpiler which transpiles text/template templates to Pure Go (it's a little coupled with my application right now, but I could always split it off and it currently misses a few features).

It would be nice, if I could reload these templates on the fly without restarting the entire instance, something impossible without the ability to close plugins or rolling my own JIT compiler.

@DemiMarie

This comment has been minimized.

Show comment
Hide comment
@DemiMarie

DemiMarie Dec 18, 2017

Here is a thought: what about having plugins export their API as a first-class object, such that the plugin gets unloaded when the API object is garbage collected?

Here is a thought: what about having plugins export their API as a first-class object, such that the plugin gets unloaded when the API object is garbage collected?

@davecheney

This comment has been minimized.

Show comment
Hide comment
@davecheney

davecheney Dec 18, 2017

Contributor

@DemiMarie this approach is problematic, please see my comments about finalisers earlier in this thread.

Contributor

davecheney commented Dec 18, 2017

@DemiMarie this approach is problematic, please see my comments about finalisers earlier in this thread.

@pjebs

This comment has been minimized.

Show comment
Hide comment
@pjebs

pjebs Apr 16, 2018

Objective C/Swift use automatic reference counting. Maybe that's the approach to take (manual book keeping - but with compiler's assistance)

pjebs commented Apr 16, 2018

Objective C/Swift use automatic reference counting. Maybe that's the approach to take (manual book keeping - but with compiler's assistance)

@pjebs

This comment has been minimized.

Show comment
Hide comment

pjebs commented Apr 20, 2018

Indirectly related for some use cases: https://www.datadoghq.com/blog/engineering/cgo-and-python/

@AlbinoGeek

This comment has been minimized.

Show comment
Hide comment
@AlbinoGeek

AlbinoGeek May 6, 2018

This whole thread seems to be discussing a non-issue

To quote the man pages on dlclose(3)

       A successful return from dlclose() does not guarantee that the
       symbols associated with handle are removed from the caller's address
       space.  In addition to references resulting from explicit dlopen()
       calls, a shared object may have been implicitly loaded (and reference
       counted) because of dependencies in other shared objects.  Only when
       all references have been released can the shared object be removed
       from the address space.

So why would we argue about trying to enforce something stronger than the underlying implementation?


Golang has no way to force you to lose your reference to something, nor do I think it should. It is and always has been you (the programmer's) responsibility to not use a resource that you have explicitly asked the system to dispose of.

AlbinoGeek commented May 6, 2018

This whole thread seems to be discussing a non-issue

To quote the man pages on dlclose(3)

       A successful return from dlclose() does not guarantee that the
       symbols associated with handle are removed from the caller's address
       space.  In addition to references resulting from explicit dlopen()
       calls, a shared object may have been implicitly loaded (and reference
       counted) because of dependencies in other shared objects.  Only when
       all references have been released can the shared object be removed
       from the address space.

So why would we argue about trying to enforce something stronger than the underlying implementation?


Golang has no way to force you to lose your reference to something, nor do I think it should. It is and always has been you (the programmer's) responsibility to not use a resource that you have explicitly asked the system to dispose of.

@tarndt

This comment has been minimized.

Show comment
Hide comment
@tarndt

tarndt May 8, 2018

Its one thing for the app to know it is no longer using references to a plugin that was loaded and think its safe to call dlclose, but what about the runtime, is the GC still going to try to scan that address space and blow up?

tarndt commented May 8, 2018

Its one thing for the app to know it is no longer using references to a plugin that was loaded and think its safe to call dlclose, but what about the runtime, is the GC still going to try to scan that address space and blow up?

@DemiMarie

This comment has been minimized.

Show comment
Hide comment
@DemiMarie

DemiMarie May 9, 2018

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