Hi,
On Mon, Aug 30, 2021 at 9:35 PM Adrien Destugues <pulkomandy@xxxxxxxxxxxxx>
wrote:
(I have not looked at your code yet, I'll do later)
I have read the full proposal now. Here are my notes as I went through it.
Probably not very clear and more questions than answers.
- Definitely don't go with both modern C++ and legacy API. It is hard
enough to get this right with one API, let's not have two completely
different ones. If you want to use modern C++, do a thing using only modern
C++. Otherwise, stay with gcc2 compatible and blend in with the existing
API.
- About the error handling: I find the Expected and BError thing a bit
strange. In existing APIs we use InitCheck and in some more recent
developments (package kit, mainly, we use exceptions). Why not use
exceptions here? It would remove the need for factory methods, you just
call the constructor and catch the exception if something goes wrong. I
think this results in clearer code, and this is how I work when doing
modern C++.
- The current design has a BUrlContext. You introduce a BUrlSession that
does largely the same thing. I think that's a naming error. A session is a
connection with a server. In HTTP 1.0, a session would have one single
request. In FTP, and in HTTP1.1 and later, a session can have multiple
requests. But still, a session is basically a TCP or SSL socket. And we
still need a BUrlContext for more global things (cookie jar for HTTP, for
example, since that can span multiple sessions).
- In section B5, I find the naming of the methods a bit confusing. My
understanding is that AddRequest actually immediately performs the request
and blocks until it is done? If so, it should be named ExecuteRequest or
something like that?
- About thread safety: I'm not sure internal locking is the way to go. It
makes sense for some things (for example the cookie jar) that are shared by
everything and require fine grained locking. But I'm not sure it makes
sense to want to use a BUrlSession (a connection to a server) accross
multiple threads. Internal locking also results in constant
locking/unlocking which isn't great for performance. I would consider
something like what we do for BView/BWindow: explicit locking, and asserts
in all methods that need a lock, to make sure it is indeed held.
- I am a bit confused by B6. Your intro says that there will be no
standard API similar to BUrlRequest, but here you proceed to describe one.
Which is it, then? This API is for example not going to work for
Bittorrent, where there would be a lot of hostname resolutions and
connections opened . Also, this does not tackle one of the problems with
the current API: some things (in particular, headers received for HTTP) are
kind of "cancellation points" for the request. Ideally, the request would
block there until the application signifies that it has processed the
headers and that the download should go on. Quite often, if the request
results in an error, there is no point in continuing and handling the
content. This has proven a bit complicated to do with the existing code.
- Section B7 says that the way to wait for a request to complete is to
call Body(), but this is specific to HTTP. Wouldn't it be nice to have a
generic way to do this for any protocol?
- B8: in the current kit, the BHttpResult is available as soon as the
headers are received. So there is an extra "request is still in progress"
phase. If we are to have independant APIs for each protocols, I think the
HTTP protocol should really follow how the requests actually work: there is
an initial phase with an exchange of data (open connection, send request +
request headers, receive status code + response headers) and then there is
an unidirectional body phase (either send or receive, depending on the
request). What to do in the body phase may depends on the headers, for
example you may need to check the content-encoding to decide if you need to
transcode the data before storing it. In the case of receiving data, it's
possible to do this after the fact, but in the case of sending data, not so
much. I think it is required to really have this 2-step process in the API.
- Part C: you would be surprised how hard it is to have an HTTP download
that "just works". Ideally it would be the case, but some websites will
lockup if you don't set an accept-language header. You also need to handle
content-encoding, you are probably interested in the MIME type of the data
you're getting (we're the one OS where we have special places for that kind
of metadata). You probably also want to manage cookies, user agent, etc.
These things would normally be in the global BUrlContext but this appears
to be gone in your proposal.
- Part C: I also have the same problem with this API as I mentionned for
the advanced one: it is not clear when the download is actually started. Is
it when creating a BUrlDownload object? Or is it when calling the Body()
method? I guess it has to be the latter, because this gives you some time
to access the prepared (but not running) request, and customize it before
it starts running.
