Change nodejs waiting on pipes to not panic #13689
Conversation
Changelog[uncommitted] (2023-09-02)Bug Fixes
|
Frassle
force-pushed
the
fraser/nodejsAwaitRun
branch
from
4f51bc1
to
ae51816
Compare
| // Channel producing the final response we want to issue to our caller. Will get the result of | ||
| // the actual nodejs process we launch, or any results caused by errors in our server/pipes. | ||
| responseChannel := make(chan *pulumirpc.RunResponse) | ||
| defer close(responseChannel) |
There was a problem hiding this comment.
This will still panic if execNodeJs finishes after pipesDone or handle.Done return an error because that'll exit this function, closing this channel, and then the goroutine will attempt to write to a closed channel, panic, and kill the process.
This can be fixed by using a channel with a buffer of one and dropping the defer close(responseChannel). Let the channel be GCed if this function returns with an error. This function is short-lived, so that's not a big deal.
Another alternative is to use the channel just to signal that execNodeJs is done, but store the value in a variable in the scope:
var response *pulumirpc.RunRespnse
haveResponse := make(chan struct{})
go func() {
defer close(haveResponse)
response = ...
}()This way, the channel will always be closed when execNodeJs finishes.
There was a problem hiding this comment.
Good spot, it looks like we can just move the close inside the goroutine to fix this as well?
| func (host *nodeLanguageHost) execNodejs(ctx context.Context, | ||
| responseChannel chan<- *pulumirpc.RunResponse, req *pulumirpc.RunRequest, |
There was a problem hiding this comment.
Good call on removing the channel from the function contract here completely.
@dixler was lamenting about something like the problem that caused this panic recently in the context of channels.
Hit this while testing with matrix testing. Fairly frequently the go routine waiting for `pipesDone` would try to write to the response channel and panic because it was already closed. This rewrites the method to only use the response channel for the running program, and we just explictly wait on all three events (pipes, handle, program) and return the first one that triggers. Also it refactors execNodejs to just be a sync method that returns the response. The management of that running in a goroutine and writing to a channel is just kept in Run.
Frassle
force-pushed
the
fraser/nodejsAwaitRun
branch
from
ae51816
to
aea8652
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Description
Hit this while testing with matrix testing. Fairly frequently the go routine waiting for
pipesDonewould try to write to the response channel and panic because it was already closed.This rewrites the method to only use the response channel for the running program, and we just explictly wait on all three events (pipes, handle, program) and return the first one that triggers.
Also it refactors execNodejs to just be a sync method that returns the response. The management of that running in a goroutine and writing to a channel is just kept in Run.
Checklist
make tidyto update any new dependenciesmake lintto verify my code passes the lint checkgofumptmake changelogand committed thechangelog/pending/<file>documenting my change