Skip to content

Conversation

@bartlomieju
Copy link
Member

Follow up #22 #49.

This PR provides most basic implementation that handles unread request body without reading all data to memory. As discussed in #49 it follows Golang's implementation.

http/server.ts Outdated
await this.w.flush();

if (!this._shouldReuseConnection()) {
this.conn.close();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ry you said that closeWriteAndWait is what we need. I don't fully understand it's purpose. Could you follow up?

@bartlomieju
Copy link
Member Author

I'll wait on #188 before finishing this

@bartlomieju
Copy link
Member Author

bartlomieju commented Feb 16, 2019

I've tried to rebase my branch against master, but given latest changes in #188 it's pretty hard.

Main loop in serve has:

for (const { req, conn } of queueToProcess) {
      if (req) {
        const res = createResponder(conn);
        yield { req, res };
      }
      serveConn(env, conn);
}

request and response are completely decoupled, which gives no easy way to check if request's body was fully read before responding, which leaves #49 unresolved

Maybe we could talk about rewriting it to something like this:

for (const { req, conn } of queueToProcess) {
      if (req) {
        // iterator should return response
        const res = yield req;

        // close connection if request body 
        // hasn't been consumed
        writeResponse(res, req);
      }

      // check if connection can be reused
      // get closed() should be added to `Conn` 
      if (!conn.closed) {
        serveConn(env, conn);
      }
}

CC @ry @kevinkassimo

@keroxp
Copy link
Contributor

keroxp commented Feb 18, 2019

@bartlomieju

Isn't code like this enough? After #188, req.body is one of instance of BodyReader or ChunkedBodyReader. It can consume body to exactly end of the stream. If body was consumed entirely by user, no additional io operations happen.

for (const { req, conn } of queueToProcess) {
  if (req) {
    const res = createResponder(conn);
    yield { req, res };
    await copy({
      write: async p => p.byteLength
    }, req.body)
  }
  serveConn(env, conn);
}

@bartlomieju
Copy link
Member Author

@keroxp agreed, however logic deciding whether or not it should be consumed or connection closed is much more complicated. We wanted to base it on Golang's implementation, here's old PR with discussion for context: #49

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

Successfully merging this pull request may close these issues.

2 participants