HttpParser review

[gregw]

Currently there is a little bit of a difference between the HttpParser contract as seen by the server and the client, specifically with how the boolean return from parseNext() is considered.

The server will call parseNext() and expect that all event handlers will have been called for the bytes that have been consumed. So for example, if the last byte consumed was the end of the headers and the end of the message, then headerComplete(), contentComplete() and messageComplete() will all have been called and their boolean returns will be or'ed together. The server never calls any application or async code from the event handlers, rather it uses them to mutate the state of HttpChannel and returns true if HttpChannel.handle should be called. This allows a parsing loop to assume that if all bytes have been consumed and if a fill returns 0, then the parser does not need to be called again.

The client on the other hand, does call application code from parser handlers, which may be asynchronous and not complete until a later time. Thus the client works on the assumption that if true is returned from any event handler, that the parser will take no further action until another call to parseNext is made. So if, for example, content(buffer) returns true, then neither contentComplete nor messageComplete will be called until a subsequent call to parseNext(). This means that the client parsing loop needs to always call the parser to try to progress - even if it has no bytes.

This issue was partially responsible for a problem with #3038, so currently the parser is implementing a fragile combination of approaches.

We need to review and go one way or the other.

[gregw]

@sbordet @lachlan-roberts This is what we were discussing in yesterdays review. I had opened and issue for it last year.

However, I'm not so sure that a one-size-fits-all parser model is actually possible. Perhaps it is best to just accept that there are two modes of usage - one where the boolean return is meaningful and another where it should be ignored?

[sbordet]

@gregw I think the main problem of calling application code from parser callbacks is the inevitable race in case the application goes async.

connection loop
  async = parse()
    parser callback
      invoke application from current parser callback
  if (async) => return

If the application listener goes async, but immediately resumes, now we have to ensure that the exiting thread does absolutely nothing other than unwinding the stack while the resuming thread enters the same stack of operations, otherwise we have a race.

Instead:

connection loop
  handle = parse()
    parser callback => changes state
  if (handle) => invoke application based on state

This reduces the race window to just the code in the connection loop, rather than the code in the connection loop and the parser and the parser callbacks and the component that invokes the application.

The client has to parse() empty buffers to advance in both approaches, so I like the latter approach better, and I don't think it will be much difficult to implement - the client already maintains a state for correspondent parser events.

[stale]

This issue has been automatically marked as stale because it has been a full year without activity. It will be closed if no further activity occurs. Thank you for your contributions.

[github-actions]

This issue has been automatically marked as stale because it has been a
full year without activity. It will be closed if no further activity occurs.
Thank you for your contributions.

[github-actions]

This issue has been automatically marked as stale because it has been a
full year without activity. It will be closed if no further activity occurs.
Thank you for your contributions.

[github-actions]

This issue has been automatically marked as stale because it has been a
full year without activity. It will be closed if no further activity occurs.
Thank you for your contributions.