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

connector-proxy: make stdin/stdout streams independent to avoid deadlocks #628

Closed
mdibaiee opened this issue Aug 10, 2022 · 5 comments
Closed
Assignees

Comments

@mdibaiee
Copy link
Member

mdibaiee commented Aug 10, 2022

Currently connector-proxy operates in this way:

  1. Keep reading from stdin (runtime sends data to stdin), translate it and send it to the underlying connector
  2. Keep reading from stdout of the underlying connector, translate ita nd send it to runtime through stdout

The issue is, the runtime waits for the stdout of connector-proxy to be closed before it closes its stdin. With our current implementation, connector-proxy likewise wants to wait until it has read all from runtime before it exits.

At the moment there is a workaround where if the underlying connector exits, we wait a few seconds and then exit connector-proxy to avoid a deadlock. However, the ideal situation is where we have stdin and stdout as independent parts.

@mdibaiee mdibaiee self-assigned this Aug 10, 2022
@mdibaiee
Copy link
Member Author

I looked into this today, apparently there is no way in Rust standard library to close stdout of the process (see rust-lang/rust#40032), however it is possible by closing fd 1 which stands for stdout. Care must be taken while doing this since when that file descriptor is closed, the next file descriptor allocation will use that descriptor, which can lead to unintended behaviour.

In this case the best practice is to close the fd and immediately request opening /dev/null on fd 1.

Note that libc::close is unsafe, so we would be introducing unsafe code in connector-proxy if we decide to proceed and implement this logic.

The code will roughly look like this (not tested):

let rc = unsafe {
  libc::freopen("/dev/null", "w", libc::fopen(libc::STDOUT_FILENO, "r"))
};
if rc == -1 {
  Ok(())
} else {
  Err(CloseError { previous: io::Error::last_os_error(), file: None })
}

@jgraettinger
Copy link
Member

If this is feeling like more trouble than it's worth, we could do an end-run around this issue by moving more directly to a world where connectors listen on a port over which they serve gRPC, and which the runtime dials.

@mdibaiee
Copy link
Member Author

@jgraettinger yeah I think that's a better approach

@mdibaiee
Copy link
Member Author

Relevant PR: #674 (comment)

@mdibaiee
Copy link
Member Author

Closing this issue since we have gone ahead and switched to TCP communications, so this is no longer applicable. #682

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

No branches or pull requests

2 participants