Skip to content

Update README.md for wrong example #1

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

futurist
Copy link

The net socket usage is wrong, see: Event: 'end'

Socket end event is for FIN package, that inform client to end connection, not for data receive end.

@watson
Copy link
Owner

watson commented Nov 19, 2017

Yeah, it will only work if the TCP connection is closed after the end of the HTTP request. If the socket is reused my example is not intended to work.

The reason why I wasn't just using the first data event was in case the headers stretched over more than one data event. But I guess that's fairly unlikely.

One thing I'd like to change in your PR is that you're using c.on() instead of c.once. And I think it would be better if the callback was just called for the first data event.

A more complete example though, that will work in all cases, would probably be if we bundled up chunks until we saw a double line-break (\r\n\r\n). That indicates the end of headers. If we saw that then we could call the httpHeaders function.

net.createServer(function (c) {
  var data = ''
  c.on('data', onData)

  function onData (chunk) {
    data += chunk.toString()

    // A double line-break indicates that we've consumed all the headers
    if (/\r\n\r\n/.test(data)) {
      c.removeListener('data', onData)

      // parse incoming data as an HTTP request and extra HTTP headers
      console.log(httpHeaders(data))      
    }
  }
}).listen(8080)

@futurist
Copy link
Author

Below code will make the example only accept ONE TIME request?

c.removeListener('data', onData)

@watson
Copy link
Owner

watson commented Nov 19, 2017

Yes. What is your use-case? Do you want to get headers for multiple HTTP requests on the same TCP connection?

@futurist
Copy link
Author

yah, that's why I need use Socket instead of HTTP, most of time http is one time connection and cannot accept another request, but in my case I need the socket keep listening and more request will come later.

@watson
Copy link
Owner

watson commented Nov 19, 2017

Ah I see. Node.js can actually be configured to use keep-alive connections. You need to use a custom agent. So you might not need this module.

But if you want to use this module, you need to write your own state logic so you know if you're inside the HTTP body or not. If you are inside the body you shouldn't try to parse it as headers. Only when you're outside of the HTTP body should you try to parse the chunks as headers 😃

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