feat(datastreams): initial impl

[anunaym14]

Have a bunch of questions added as comments, please check those!

TODO:
- Docs ✅
- Attachments for text streams ✅
- Sanitize example ✅
- Removing deprecated DataPacket.Kind usage ✅
- #622 (comment) ✅

[boks1971]

LGTM! But, I do not understand the nuances of the e2e flow and the protocol. Left some comments.

[boks1971]

just return immediately if nil? Haven't read the full code to understand the use for this. Maybe, returning immediately is not the right thing, but seems like valid behaviour if dc can be nil.

[boks1971]

Actually return an error and bubble that up all the way?

[anunaym14]

Actually return an error and bubble that up all the way?

Yeah I was thinking the same

[anunaym14]

Done, but error propagation is a bit tricky, please check the newly added comments

[anunaym14]

Thinking about this again, we should not return an error if the datachannel isn't available, instead we should wait for it to come up, then all the publish calls won't have any reason to fail either, so we can get rid of the error propagation and call this in a goroutine internally.

The above ideology fails if a data channel closes unexpectedly, in that case, do we open a new one?

[boks1971]

opening a new data channel is an option, but we do not do it anywhere now AFAIK. So, should make sure that server can handle it properly.

[anunaym14]

@boks1971 please check this again, I'll replied to some of the comments and marked others which are done as resolved. Feel free to mark them as unresolved if you think something needs a change

[boks1971]

Thank you for addressing comments @anunaym14 . I think there are still bits that need some clarification. Have left some comments.

[boks1971]

Are you leaving this to the user? I thought we could just do it internally and externally, the user either gets data or io.EOF. WDYT?

[anunaym14]

We need ErrAgain anyway, because, if there are some bytes to read, but not enough, the Go APIs will return the available data with an EOF. In that case, before stream close, we'll have to notify that to the user with ErrAgain (better to return what we have than waiting for more). And since, the user will be handling ErrAgain anyway for that case, it might make sense to let them handle it for no bytes available case as well.

But I understand the pros of doing it internally as well, I don't really have a hard preference among the two.

[boks1971]

Understood, but feels like it can be handled internally.

notify that to the user with ErrAgain (better to return what we have than waiting for more)
I am not understanding. I think this can return (num_bytes_read, nil) and user would take those bytes and read again.

Internally, if we get (o, io.EOF) from Go APIs before the stream end, we just loop and go back and block on read?

[anunaym14]

So block internally just on (0, io.EOF)?

In cases of (n, io.EOF) where n > 0, do we return ErrAgain with n? Will modify the error message to day "not enough bytes available"

[boks1971]

Maybe my understanding is incomplete, but this what I am thinking

user

for {
    if chunk, err := stream.Read(); err != nil {
      // done
      break
    } else {
       data += chunk
    }

internally

func Read() ([]byte, error) {
     buf, err := buffer.Read()
     // unclear to me how the end of stream is known, assuming that is known
     if end-of-stream {
          return buf, io.EOF
     }. else if len(buf) > 0 {
         return buf, nil
     } else {
          // len(buf) == 0 and err is probably io.EOF
          time.Sleep(10 * time.Millisecond)
     }
}

Effectively, Read() blocks till there is data or end of stream.

[anunaym14]

Understood. Just one doubt:

The Read() method takes []byte say p and returns len(p) bytes from the buffer, if buffer has less bytes, it returns whatever is available with err == nil and only returns io.EOF when the buffer has nothing to read from.

But, the ReadBytes() method takes a delim byte, and returns till the delim including the delim itself. But, if delim isn't present in the buffer, it returns everything available but with io.EOF.

How should we handle this? Do we return nil in these cases (before stream closure), or do we return either or ErrAgain or io.EOF?

[boks1971]

I think io.EOF should be returned only on stream end. As long we know the stream end and can return io.EOF only on that, that is clear.

For other cases

• Read() has data, internally get (n, nil) -> pass that on to the user

• Read() has no data, internally get (0, io.EOF) -> internally we know this is not end of stream, so loop and read again

• ReadBytes() has delim - internally get (n, nil) -> pass that on to the user

• ReadBytes() has no delim - internally get (n, i.EOF) -> this seems to be only tricky case -> I think we should return (n, nil) to the user, the reason I am saying nil here is ErrAgain or nil does not make a difference to the user. They just have to read till stream end. So, what I am proposing to not return an extra error unless there is some well-defined clear use that is not covered by other paths.

[anunaym14]

Please check the latest commit, this should be handled properly now!