[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: