-
Notifications
You must be signed in to change notification settings - Fork 52
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(external): close stdin properly in shutdown() #255
Conversation
Signed-off-by: Li Yazhou <[email protected]>
d2578b1
to
0c568cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix! Just curious where are you using the external engine?
Do you want to bump version (and CHANGELOG) now? I can publish a new release for you. |
i am trying to maintain a small set of a smoke test suite for my cloud service, this test suite is not planned to be open sourced and won't become big, so i guess it might be a good choice to use the |
thank you a lot! let me update it : ) |
Signed-off-by: Li Yazhou <[email protected]>
b7342ae
to
2cc77ab
Compare
self.stdin.shutdown().await.ok(); | ||
drop(self.stdin.take()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just found the implementation of shutdown
for ChildStdio
is just Poll::Ready(Ok(()))
. So the problem is drop
not called, not because of shutdown
called.
sqllogictest-engines/src/external.rs
Outdated
match &mut self.stdin { | ||
Some(stdin) => stdin.write_all(input.as_bytes()).await?, | ||
None => return Err(io::Error::from(io::ErrorKind::UnexpectedEof).into()), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we directly panic if it's already shut down, just like
&self.conn.as_ref().expect("connection is shutdown").0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sense, updated it in 6a05348, PTAL
Signed-off-by: Li Yazhou <[email protected]>
Signed-off-by: xxchan <[email protected]>
0.27.2 published |
it seems that we can not pass EOF to child process's stdin by
shutdown()
.as the doc from tokio described,
shutdown()
will fire ashutdown(Write)
syscall to the socket. but this syscall does not works for file descriptors other than socket.to real
close()
the fd to pass the eof properly to the child process, we have todrop()
this stdin to close it.i made an experiment in: https://gist.github.com/flaneur2020/4e74d7c8acc6b8dbe41cacb28c131dba . this program will keep hangs with
shutdown()
, and will runs as expected withdrop()
.this pr fixed the
shutdown()
todrop()
to close the file descriptor properly.