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

  • From: Niels Sascha Reedijk <niels.reedijk@xxxxxxxxx>
  • To: Haiku Development <haiku-development@xxxxxxxxxxxxx>
  • Date: Wed, 1 Sep 2021 07:13:06 +0100

Hi,

On Mon, Aug 30, 2021 at 8:42 PM Mr. waddlesplash <waddlesplash@xxxxxxxxx>
wrote:

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).

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.


This would be the BUrlDownload in section C. The question here is whether
we go for the absolute minimum (here is a URL, give me the data), or
whether we want to implement some features that may be common to multiple
protocols (like the range). I have no strong opinion on either.



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.

(...)

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


I think there is an interesting discussion to be had here, but that it is
probably going to be worth breaking down the discussion into concrete
individual proposals on which modern C++ features we would use (and how) in
public APIs and their implementations, and how they work with APIs that
have been designed with the C++98 feature set.

I will put in a PR on Github with a document that will break down some
features and that we can then comment on line by line, so that we can see
where we stand in the community.


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.)


I try to approach it from the angle of how to prevent the most harm. Since
BDataIO itself does not do or prescribe any locking, they are not
thread-safe objects, and as such there is an inherent danger of abuse when
two threads concurrently can access it. Furthermore, there's the question
of who cleans it up when the request is cancelled prematurely.

Wrapping a BDataIO in a unique_ptr<> has the following implications:
 * Before you launch the request, you own it. Once you add/schedule the
request, ownership (and responsibility for cleanup) will go to the Network
Services Kit.
 * In case you want to cancel the request, you have two options. If you do
not care about the BDataIO object, you just let your response object go out
of scope. It will be cleaned up by BHttpSession then. If you do want it
back, you can explicitly cancel the request at the session and take back
the BDataIO object from the response object you are holding.

Obviously this does not prevent anyone from keeping a copy of the pointer
(std::unique_ptr<>::get()) and doing stupid things, but my opinion is that
the API at least did its best to communicate the intent.


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?


Another design choice: I strongly believe that scoped objects are less
dangerous both in terms of memory leaks, as well as concurrency (ab)use.
BHttpResult objects will be non-copyable/movable types. If you receive a
message, you can check if you hold the BHttpResult object. If so, act on
it, if not, you may have passively cancelled the request (by dropping the
object out of scope) or you may be listening in the wrong place.

(Again, it is true that there is nothing stopping one from circumventing
the 'unique' object by spreading references/pointers around, but that would
be someone shooting themselves in the foot).

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.


I think it is not untenable (and if I have named it untenable, I was
exaggerating), but it is not useful. In the proposed design I have moved
the logic of protocol execution to the B*Session classes. This means that
the B*Request objects are nothing more than data structures for the
properties of the request. If you would boil down a base BUrlRequest that
would be the common parent for all of them, you would really be left with
something that holds a BUrl. Why not have BUrlDownload just accept a URL.

The calculation on this might change, especially if we want to get some
more functionality as mentioned at the beginning of your email, like
supporting ranges.

I hope this clarifies the intention somewhat.

Regards,

N>

Other related posts: