[haiku-development] Re: Plan for NetServices Kit (v2)
- From: "Adrien Destugues" <pulkomandy@xxxxxxxxxxxxx>
- To: haiku-development@xxxxxxxxxxxxx
- Date: Mon, 30 Aug 2021 20:31:31 +0000
(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.
--
Adrien / PulkoMandy.
Other related posts:
