[haiku-development] Re: Plan for NetServices Kit (v2)

  • From: "Mr. waddlesplash" <waddlesplash@xxxxxxxxx>
  • To: haiku-development@xxxxxxxxxxxxx
  • Date: Mon, 30 Aug 2021 15:42:10 -0400

Overall, I think this is going very much in the right direction, even
if I have some sharply critical notes in here on your suggestions in
particular. Thanks and kudos for working on this!

On Mon, Aug 30, 2021 at 1:14 AM Niels Sascha Reedijk
<niels.reedijk@xxxxxxxxx> wrote:

In my opinion, if we want to continue to provide a Haiku-centric way of 
performing network requests with multiple protocols, then we should (1) 
provide specific implementations for each of the protocols (BHttpSession, 
BGopherSession, BFtpSession) and (2) an easy API to just 'get data at a URL' 
for protocols that support an easy download (BUrlDownload).

I think it makes sense to have a single B...Session class and not
many, because there may be instances were protocols interact with each
other (for instance, WebSockets is not HTTP but has defined
interactions with it; also, HTTP/1, /2 and /3 are technically separate
protocols that share a name and a broad array of purposes.)

We should probably have a BUrlRequest that can handle basic
download/upload support in a protocol-independent manner, yes; whether
it looks anything like the current BUrlRequest is another matter.
Hopefully it could also support e.g. range requests which are common
to HTTP and FTP for instance.

I also concur with Adrien's comments here.

2. Then there is another discussion of how to implement this in the light of 
modern C++. The GCC 8.3 compiler has pretty complete C++17 support, and while 
I imagine there are different feelings about modern C++ and the direction it 
is going, there is no denying it is here and that it makes sense to start 
thinking and discussing what it means for Haiku. Initially my thought would 
be to make the new API available both for the x86_gcc2 platform and the x86 
platform, but I am starting to change my mind... I actually think I would 
like to keep the support on x86_gcc2 to a minimum (up to the level that is 
needed for the package kit), and really go to places on the modern compiler. 
This will make it so that we can take advantage of move semantics, smart 
pointers, constexpr, some metaprogramming and new features in the standard 
library.

As I mentioned to you before on IRC: to be honest, I think we should
avoid exposing any of this to the public API for now. We have
BReferenceable for reference-counting objects and certainly we can
make more use of that as it makes sense to, and of course we can use
our own smart pointers (ObjectDeleter, etc.) internally; but I think
we should avoid any drastically different API surfaces here, at least
at first. That is to say, designing the network kit itself is kind of
an experiment, let's avoid doing two or three experiments at once and
stick to just one.

It is my personal opinion at least that a lot of the newer C++
features may have good motivations but their actual design and
implementation ranges from suboptimal to disastrous. I would prefer we
carefully review any features from it before adopting them, or (more
likely) to just reinvent classes and features rather than adopting the
STL ones. I also note that C++ still does not really have concepts of
"lifetimes" the way Rust or other "explicit non-GC memory safety"
languages do, and a lot of these new features might wind up with very
different implementations or designs when a wholistic memory
management strategy (like Haiku's toolkits should, in theory, have) is
defined.

If you want to experiment with using such features internally, in a
way that public headers do not actually expose or require the use of
any of it, then that may be OK, I guess, depending on how it is done.
But it is also the case that, so far as I can tell, a lot of the
network kit should have relatively straightforward lifetimes and
memory management semantics, so how much of this is actually needed or
useful in this context is in my mind an open question.

(Some of the examples you have of move semantics being used, e.g.
network cookies, seems to me to be totally the opposite of what we
usually do in Be/Haiku APIs; for example, BStrings being added to
BMessages are copied and not moved, I would expect cookies being added
to a cookie jar/session to behave very similarly to adding keys/values
to a BMessage. Even if ultimately this design is more in line with how
other APIs do things (and to my knowledge of e.g. Qt's APIs, I don't
think it is), we should retain consistency even if there is a slight
decrease in efficiency here.)

So, in short, I disagree strongly with section A.4 in your present proposal.

3. And the final part is about how to approach this. This will obviously be a 
lot of work, and I do not want to overload regular code review. There is also 
a question about how we vet and validate the internal design. The options are:
(a) keep working in a separate repository, implement the gaps, do testing and 
error checking, and when there is a minimum definition of done, then merge 
and refine it on the main repository
(b) go for a feature branch in the Haiku repository so that we can use 
regular methods of peer review to make it progress properly
(c) work in a `netservices2` directory structure in the master branch and 
continue from there.

I would vote for (a). We have never really done (b) before or at least
not since migrating to Git; all major "feature branches" previously
lived in personal GitHub forks (most notably, package management.) I
guess (c) could be fine too and not without precedent.

Now on to commenting on the rest of your proposal:

B.4 Scheduling a HTTP Request

As BMemoryRingIO is a BDataIO, I think that some work should be put in
here to try and ensure that there are no special semantics for it,
leaving open the possibility that as long as certain requirements and
guarantees are met, applications can provide their own DataIO classes
which do something similar to what BMemoryRingIO does internally.

I am also not sure about the semantics you are proposing for BDataIO
ownership. I don't think having the request take ownership of the
BDataIO makes much sense. Just requiring that the application must not
destroy the BDataIO before either cancelling or deleting the request
makes the most sense to me. (Or, at least having an option by which
the request does not take ownership.)

B.6 Asynchronous Handling of the Result

What is the purpose of the IDs, and how will they be allocated? What
is the advantage over having IDs instead of just treating the pointer
to the BUrlRequest or equivalent as the unique identifier for the
request?

Part C: Introducing BUrlDownload

I guess I would be interested to know what precisely makes the current
design, where e.g. a BHttpRequest inherits from a BUrlRequest, and the
most generic class can be used to make generic requests, untenable.
(The current design may well be untenable, I speak here only of the
basic idea that BHttpRequest would inherit from some lower
BUrlRequest.) That would then avoid having this "parallel API" design
entirely.

Thanks once again for spending the time on this!
-waddlesplash

Other related posts: