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

Handle unread body from request #186

Closed
wants to merge 7 commits into from
Closed

Handle unread body from request #186

wants to merge 7 commits into from

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
@@ -271,6 +340,10 @@ export class ServerRequest {
}

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