xtaci / smux

A Stream Multiplexing Library for golang with least memory usage(TDMA)
MIT License
1.31k stars 196 forks source link

Reads on all streams fail after a single read error #61

Closed joesis closed 4 years ago

joesis commented 4 years ago

See https://gist.github.com/joesis/340e21f44cc65e2119668d116f397e49 for a demo. Once the client TCP connection hits read deadline (just to demonstrate. Whichever temporary error could trigger the problem), the session signals socket read error which is persisted so later Read on all streams will fail, but OpenStream and Write on the streams can still work, until the session being closed by the keep alive timer. From the server side, everything works as if there's no problem at all.

I don't think the caller is expected to close the session when seeing a single read error on one stream, which can be specific to the stream and not affecting others, so such errors should be at least visible on the session level. I would even prefer close the session as it enters into an irrecoverable state.

Thoughts?

xtaci commented 4 years ago

How about half-closed net.Conn? peer can write only.

joesis commented 4 years ago

@xtaci Do you mean session or stream? If a session is half closed, are new streams supposed to be open over it?

xtaci commented 4 years ago

the behavior is exactly the same as syscall read(2) write(2), if read/write returns error on file descriptor, it's the caller's responsibility to close the file descriptor, what do you think?

xtaci commented 4 years ago

OpenStream will fail if it cannot write()

joesis commented 4 years ago

the behavior is exactly the same as syscall read(2) write(2), if read/write returns error on file descriptor, it's the caller's responsibility to close the file descriptor, what do you think?

Yeah surely the caller should close stream it it sees read/write error, but it won't know that new steams are doomed to have read/write errors.

Having OpenStream returning error in such case sounds good to me.

xtaci commented 4 years ago

sounds good, if OpenStream returns false

xtaci commented 4 years ago

https://github.com/xtaci/smux/releases/tag/v1.5.10

joesis commented 4 years ago

Thanks @xtaci !!!