HTTPClient cannot perform multiple requests

[CandleCandle]

There appears no way to re-use a HTTPClient despite the documentation suggesting that one should create one HTTPClient and then re-using it.

The HandlerMaker's apply method is only called once when the HttpSession is created and then stored against the (scheme, host, port) tuple, thus when the second request is made, it uses the Handler from the first request.

Code that reproduces the issue & a more in-depth discussion of the problem:
https://gist.github.com/CandleCandle/260a284ece6d2e99520cfbd99798e739

[pdtwonotes]

You correctly understand what it is doing. However, it behaves that way on purpose. The Handler is related to the connection, not to the request. A connection can deal with multiple requests as this saves the time of tearing down and re-establishing the TCPConnection each time. In certain cases it is even possible to pipeline requests, with some restrictions, and if the particular server chooses to support that.

So there is a certain amount of context associated with the connection (like if the server end closes it, backpressure, and so on) and a certain amount of context associated with each individual request. Your HandlerMaker is called only when a new connection is created. On the second request the apply method of the first Handler should be called, since it is using the same connection.

[CandleCandle]

From a usability perspective, the signature:

HTTPClient.apply(request: Payload trn, handlermaker: HandlerFactory val): Payload val ?

implies that the handler is associated with the payload, this is misleading because the handler factory is completely ignored for subsequent requests.

In the API abstraction layer, you would need to maintain a collection of sent payloads, and have the handler call a behaviour of the abstraction layer, where you would need to match the response with the sent request. Given the signature of HTTPClient.apply this feels like it should be the responsibility of the HTTPClient.

I entirely support TCPConnection reuse but it looks like the concepts of connection pooling and response handling have been merged, leading to the difficulty in using the HTTPClient.

Are you able to provide a simple example where the same client is used to perform multiple different requests to the same host, with each doing something different on receiving the response?

[pdtwonotes]

In addition to just looking at the signature of the apply method, you need to read the extensive documentation in file http_handler.pony that explains what the Handler is for and when it gets created. This turns up in the auto-generated package documentation as well.

Yes, if you need to distinguish between responses, you would need a Map of pending sent Requests. But some of the things coming back to the Handler have to do with the underlying TCPConnection, not individual Requests. I did not want to muddy things further by having two handlers. The Handler deals with the Session and the HTTP protocol - that is why the interface name is HTTPHandler not HTTPRequestHandler.

This is not a bug exactly, but a philosophical issue about what the API should look like. It does not have to be the way it is. I would like to hear more discussion on this.

[peterzandbergen]

I would like to add to the discussion and propose a change that I implemented.

I made the handler factory part of the identification of the _HostService, so now a new _ClientConnection is created when the HandlerFactory is different for the same scheme, host and port. This at least gives a better relationship between the apply accepting the handler factory as a parameter and the handler factory being used.

Another change I want to propose concerns a bug in the code handling the payloads. This is the code in the _ClientConnection. In the current implementation sending two requests to the same _ConnectionHandler fails with the two payloads not being handled at all. After some searching I found the error in the code that determines whether or not the var _conn: (TCPConnection | None) is a valid, opened connection.

The following scenario plays whent the HTTPClient apply is called with two payloads for the same _ClientConnection. This results in two apply behaviours being called which are then processed by the _ClientConnection asynchronously.

This is the scenario:

1. be apply is called with payload 1
2. apply pushes payload 1 on the _unsent list
3. _send_pending is called
4. _send_pending tries to cast _conn as TCPConnection and fails, because _conn is None; it calls _new_conn()
5. _new_conn creates a new TCPConnection and assigns it to _conn
6. be apply returns
7. be apply is called with payload 2
8. apply pushes the payload on the _unsent list which now contains two payloads
9. apply calls _send_pending
10. _send_pending send_pending tries to cast _conn as TCPConnection and succeeds, but _conn is not open yet
11. _send_pending continues to shift the payloads from the _unsent list and calls request.write, which it thinks succeeds.
12. after all the request have been processed _send_pending and apply return and some time after that the be _connected behaviour is called and now _conn is really open and ready for use.

It works when you only send one payload, because _send_pending is called again after be _connected is called and _conn is a connected TCPConnection.

This is the scenario:

1. be apply is called with payload 1
2. apply pushes payload 1 on the _unsent list
3. _send_pending is called
4. _send_pending tries to cast _conn as TCPConnection and fails, because _conn is None; it calls _new_conn()
5. _new_conn creates a new TCPConnection and assigns it to _conn
6. be _connected is called and calls _send_pending
7. _send_pending works fine now, because _conn is actually connected.

The fix was easy: I changed the type of _conn and gave it an extra type _ConnConnecting

primitive _ConnConnecting
...
var _conn: (TCPConnection | _ConnConnecting | None) = None

The changed _new_conn() tests if it is connecting and returns if it is.

if _conn is _ConnConnecting then
  return
end

The behaviour _connected sets _conn to the received TCPConnection

be _connected(conn: TCPConnection) =>
  _conn = conn
  // rest is unchanged

[peterzandbergen]

I can send in the fix but could some help in preparing a pull request.