socket closes too early on sync writes

[dignifiedquire]

When running a server in node like this

const createServer = require('pull-ws/server')
const pull = require('pull-stream')

createServer((stream) => {
  pull(stream, stream)
}).listen(5555)

and a client like this

pull(
  pull.infinite(),
  pull.take(100),
  // delay(0),
  connect('ws://localhost:5555'),
  pull.collect((err, result) => {
    if (err) throw err
    assert(result.length === 100)
  })
)

function delay (n) {
  return (read) => (err, cb) => {
    if (err) return read(err)
    setTimeout(() => {
      read(null, cb)
    }, n)
  }
}

It fails, with the result being equal to only the first element in the series. If I uncomment the delay it works as expected.
I took the delay definition out of the tests in here, so it seems this is somewhat known, but it still is a problem for me.
I looked at the traffic in the browser and all things are sent and received, just the pull-stream itself is aborted without forwarding all the messages.

[dignifiedquire]

After some investigation I found that the reason this is happening is because the client closes the socket as soon as it's gets the end from the .take(100). Resulting in the connection being closed before all messages have been received from the server.

[dominictarr]

yeah, tcp streams have the allowHalfOpen feature
https://nodejs.org/docs/latest/api/net.html#net_net_createserver_options_connectionlistener

but websockets don't have a thing like that. the trouble is that websockets don't have a kind of "end" message, so you (unfortunately) need to add something to the protocol. It's exactly like a telephone,
either party can hangup, or the call can be cut off. you don't really know whether they have heard you properly or not, so the solution is to say "goodbye" then wait to hear their goodbye before you hang up.

there is a module for that! https://github.com/dominictarr/pull-goodbye

you could use that, or, in this case, the bulk encryption protocol (for the body of the session, after the handshake) ought to have a way to signal a correct end, for example pull-box-stream ends in a zero length packet https://github.com/dominictarr/pull-box-stream#protocol-1

[dominictarr]

sorry, I didn't read your post properly - this is just about ensuring the data is actually sent...
maybe we ought to wait until bufferedAmount is zero?
https://developer.mozilla.org/en-US/docs/Web/API/WebSocket

there is no callback for that, though, so we'd have to poll it );

[dignifiedquire]

Thanks a lot @dominictarr! My issue was with both things you mentioned, the first problem with the "halfOpen" connection and the other with concern about ensuring the data is actually sent before we close the connection.
It might be though that it's fine what we are doing with respect to closing before all is sent, I need to investigate what exactly the WebSocket does when there is still data in buffered and socket.close() is called, it might be already doing the right thing.

[daviddias]

This issue is similar to the one we have in utp, since libutp doesn't implement the FIN packet described on the spec, which disables the transport to be halfopen.

go-ipfs has been (historically) just blowing the full connection and handling the result of that as a regular error (i.e lack of connectivity). This doesn't work as nicely for JS, because it just causes errors when they don't have to happen and with spdy on top.

I really much prefer to close gracefully a connection if we can do it in that way. To implement that, we need to add a special 'packet' to signal the FIN, which can happen in one of two ways (AFAIK):

• We wrap every packet and add an header - Adds at least 1 byte overhead to every message
• We decide on a sequence of bytes that represents a FIN - If a message happens to have that sequence, the connection gets closed and the user doesn't understand why

In addition to this, every other WebSockets language implementation will need this to be compatible with libp2p, so at that point, better write an RFC for a WebSockets extension.

I like the idea of the simple extension and wrapping the packets. Note: it doesn't have to happen on pull-ws, just at libp2p-websockets, as we don't want to force every WebSockets and pull-streams user to do it :).

[whyrusleeping]

@diasdavid it really just sounds like libutp needs to implement the spec fully. I'm not on board with wrapping packets and adding a header, the degree of waste in the protocol already with wrapping outgoing data is rather immense. ( The discrepancy described here ipfs/kubo#1040 is the result of that overhead)

[dignifiedquire]

@whyrusleeping that does not answer the question about what to do with websockets though

[whyrusleeping]

@dignifiedquire hrm, i'm not really sure if i have any valuable input here. in go when you write, you will get an error if the write failed. You will also fail to write if the other side has closed their end of the connection.

[dominictarr]

websockets already have framing - would it be possible to just send a zero length message? (just an idea, I am not sure whether websockets let you do this)