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

Send the "server listening" message only after the server has started #1173

Closed
yoshuawuyts opened this issue Oct 2, 2020 · 5 comments · Fixed by #1367
Closed

Send the "server listening" message only after the server has started #1173

yoshuawuyts opened this issue Oct 2, 2020 · 5 comments · Fixed by #1367
Assignees
Labels
kind/bug A reported bug.
Milestone

Comments

@yoshuawuyts
Copy link
Contributor

@timsuchanek reported we were emitting the "server listening" message before the server has started. This requires a retry loop on client to work around. We should fix this in Tide instead by adding a listen_with method that invokes a callback after the server has started. I've filed http-rs/tide#712 for that purpose.

@yoshuawuyts
Copy link
Contributor Author

http-rs/tide#724 has been filed which implements Server::listen_with. The steps after this are:

  • merge and release a tide version with this
  • integrate into engines
  • remove the retry hack from client

The code for engines should be as easy as:

After

    use tide::connection::ConnectionInfo;

    // Create the log message used by `prisma-client` to connect
    let reporter = |info: ConnectionInfo| async move {
        info!("Started http server on {}", info.connection());
        Ok(())
    };

    // Start the Tide server and log the server details.
    if let Some(path) = opts.unix_path() {
        app.listen_with(&*format!("http+unix://{}", path), reporter).await?;
    } else {
        app.listen_with((&*opts.host, opts.port), reporter).await?;
    }

Before

    // NOTE: This println is essential for the correct working of the client.
    info!("Started http server");

    // Start the Tide server and log the server details.
    // TODO: Tide should have a panicking listen_unix impl.
    if let Some(path) = opts.unix_path() {
        app.listen(&*format!("http+unix://{}", path)).await?;
    } else {
        app.listen((&*opts.host, opts.port)).await?;
    }

@yoshuawuyts
Copy link
Contributor Author

Update for the retro: the PR on Tide has not been merged yet, which blocks progress on this. We can carry this over.

@janpio
Copy link
Member

janpio commented Oct 27, 2020

There are also review comments on the PR that might need work or answers.

@yoshuawuyts
Copy link
Contributor Author

@janpio yes good point; picked that back up again.

@janpio janpio modified the milestones: Backlog 2.10.0, Backlog 2.11.0 Oct 28, 2020
@timsuchanek timsuchanek modified the milestones: Backlog 2.11.0, Backlog 2.12.0 Nov 11, 2020
@matthewmueller matthewmueller modified the milestones: Backlog 2.12.0, Release 2.12.0 Nov 11, 2020
@yoshuawuyts
Copy link
Contributor Author

Filed http-rs/tide#740; once that's merged we can pick the patch back up on Prisma's side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A reported bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants