vibe-d / vibe-http

Future vibe.d HTTP implementation
15 stars 9 forks source link

Porting of http1 with scoped API and waitForDataAsync #2

Closed GallaFrancesco closed 6 years ago

GallaFrancesco commented 6 years ago

We are proposing this PR to port some code from the old vibe.http we think we should mantain and to start introducing some changes.

The PR involves only two, pretty heavy commits to allow for better understanding of the different changes we had to make:

One issue we would like to point out is that we had to modify vibe-core and remove the default destructor for TCPConnection. This requires manually closing the connection if a timeout expires or an error occours, but otherwise the scoped destruction would have prevented its usage inside a closure.

The current implementation of vibe-core results in an error:

http1.d(66,14): Error: variable vibe.http.internal.http1.handleHTTP1Request!(TCPConnection).handleHTTP1Request.connection has scoped destruction, cannot build closure 

The modified implementation can be found at https://github.com/GallaFrancesco/vibe-core

s-ludwig commented 6 years ago

Sorry for the late reply, I missed the notification for this PR at the time and @wilzbach just mentioned this to me. I will try to review the changes in the next days (maybe tomorrow while in the train).

As for listenHTTP, what about providing two overloads that take the handler through a parameter with the concrete type to retain backwards compatibility: listenHTTP(H)(..., H handler) if (is(typeof(handler.handleRequest(HTTPServerRequest.init, HTTPServerResponse.init))); and listenHTTP(C)(..., C handler_callback) if (is(typeof(handler_callback(HTTPServerRequest.init, HTTPServerResponse.init)));

I'll also set up basic Travis-CI to keep track of compiler errors.

FraMecca commented 6 years ago

Thank you @s-ludwig , we'll wait for your feedback in the next days.

GallaFrancesco commented 6 years ago

Thank you for the review. Unfortunately we have little time to work on this mainly because of university deadlines. If it's ok for you we would like to take a couple of weeks and then resume working on this PR.

s-ludwig commented 6 years ago

No problem at all! Since I also have very limited time available right now, that actually makes it more comfortable to handle. I'll try to follow up with a PR that adresses the biggest changes that I had in mind then, so that we can get on a more "constructive" track as soon as possible, concentrating on the actual additions/improvements.

GallaFrancesco commented 6 years ago

@s-ludwig sorry for the delay. We started by fixing some parts of your review. Before we proceed any further we would like some clarifications about the rationale behind the allocationless handler. We are considering two possible scenarios:

  1. doing a complete rewrite of HandleHTTP1Request by moving many parts of server.d in http1.d and pushing the allocations somewhere else (where?)
  2. following the hints on the wiki and writing an api that uses HTTPInterchange (that was not the original indication as of the discussion on the forum)

Regarding 2. it would be difficult for us to imagine building an high level api that is coherent with the previous one, with an additional layer that provides backward compatibility, even though it seems a better option if you take into consideration the integration with http2 and decoupling the logic from allocations.