[haiku-development] Re: API Design: BUrlRequest and BDataIO

  • From: Niels Sascha Reedijk <niels.reedijk@xxxxxxxxx>
  • To: Haiku Development <haiku-development@xxxxxxxxxxxxx>
  • Date: Sun, 14 Feb 2021 09:40:17 +0000

Hi,

On Sat, Feb 13, 2021 at 5:33 PM Adrien Destugues
<pulkomandy@xxxxxxxxxxxxx> wrote:


So that answers the first part of my concern, but I still wonder
whether the overall ergonomics of the API are the best. Because
basically we put the onus on the user of the API to build in locking
while the request is running.

Why would we not make the B*Request objects itself implement a Read()
method? These can be called in the hook functions in the listeners to
'stream' the file/data into the definitive output. It just seems to me
like there will be fewer accidents this way.

I think it just makes things more complicated.

The current API is simple for the simple case. If you want to download
something to a file, you pass a BFile. If you want to download something
to memory, you pass a BDataIO. If you want to download something and do
processing "on the fly" (streaming, for example), you use the new
BMemoryRingIO class which provides the locking. The "download to memory"
and "download to file" cases already cover 90% of uses of the API. The
BMemoryRingIO probably covers 90% of the remaining ones.

I get the motivation.

I am just slightly more worried about the 'user manual' to consumers of the API.

The sunny day scenario is:
1. In the part of the app that creates the request (let's call it a
controller), you need to set up the data target (implementing the
BDataIO interface).
2. When you create the B*Request, you exclusively borrow the data
store to this request. This means that you cannot use it (unless it is
a thread-safe implementation like BMemoryIO), but you still own it.
3. When the request is done, then you get the object back and you can
hold it again.

I think the API itself does not communicate rule #2 very well,
inviting race conditions or other undefined behaviour. Obviously we
can communicate this in API documentation, but it can be easily
missed/misunderstood.

The rainy day scenario is slightly more worrying. Let's say the
controller is quit while a network request is running. The cleanup
routine needs to stop the request and cleanup the allocated BDataIO.
B*Request::Stop() is not a blocking function, instead it gives an
indication to its implementation that the loop should be ending.
However, there is still a chance of a data race between the protocol
loop ending and the BDataIO * being cleaned up. The only alternative I
can think of is having a while (!request->IsRunning()) after issuing
the Stop command.

Or am I overlooking some synchronisation primitives in this case?

N>



So, not only this approach is the simplest, it is also the most flexible
(since you can plug any BDataIO you want) and the most efficient (no extra
copies of the data, no need to juggle the data between multiple threads).

Finally, it is hard to say what will be needed by applications in that
remaining 1% of cases. Would they prefer to be notified for the available
data from a BMessage? Using a pthread lock? a semaphore? A file descriptor
they can use select() on? A blocking read from a BDataIO? It's impossible
to know. So, all we could do to manage threading is provide one interface
which may not cover all cases. That's what BMemoryRingIO does, but it does
it in a way that it is entirely optional, and can be replaced with something
else where needed.

--
Adrien.



Other related posts: