Add message framing to support half-close and reset of stream #20
Conversation
|
Great to see this. I am curious what you think of the proposed framing mechanism. We need it for the browser side, thus we should adjust it to the needs of this JS implementation. |
|
As an aside, this diff will become cleaner when #19 is merged into develop, since there are changes in common. @mxinden I haven't yet run into anything with this mechanism that is specially/unusually difficult to implement on the browser side. I had misunderstood the discussion of the varint length prefix at first, so perhaps we could at some point attempt to add some clarity for newbies' sake. But then again, I clearly haven't finished. In particular I haven't attempted to handle flags being sent to the browser, which conceptually should be roughly half the proposal. |
John-LittleBearLabs
force-pushed
the
jt/framing
branch
from
998b955 to
dc2042f
Compare
src/stream.ts
Outdated
| } | ||
|
|
||
| private _sendFlag(flag: pb.Message_Flag): void { | ||
| if (this.writeClosed) { |
There was a problem hiding this comment.
we should still be able to send a flag if write is closed. This is useful in case of stream resets.
…on change to the spec PR.
| @@ -7,6 +7,8 @@ | |||
| "license": "Apache-2.0 or MIT", | |||
| "type": "module", | |||
| "scripts": { | |||
| "autogen": "npx protoc --ts_out proto_ts --proto_path src src/*.proto", | |||
There was a problem hiding this comment.
Can you document these in the README?
src/message.proto
Outdated
| RESET = 2; | ||
| } | ||
|
|
||
| optional Flag flag=1; |
There was a problem hiding this comment.
nit: can you make the spacing consistent with the below flag = 1;
| break; | ||
| case pb.Message_Flag.RESET: | ||
| log.trace('Remote abruptly stopped sending, indicated with "RESET" flag.'); | ||
| this.closeRead(); |
There was a problem hiding this comment.
Is there the possibility that this isn't exhaustive and needs a default case?
There was a problem hiding this comment.
What would a default case do? Throw an exception?
There was a problem hiding this comment.
I don't know enough of the context to know, but I guess the undefined case is acting like the default case. I was just thinking of the possibility of other values not here, like null or whatever else is possible. I think you can ignore this.
There was a problem hiding this comment.
undefined is there because the flag is optional in the protobuf. If it's missing it means it's a normal packet doing normal streaming things for the application (the protocol allows for the flag and message to both be present, but in practice it's one or the other).
In Javascript if it's a value that's not handled and there's no default, does it skip the whole switch like it would in C ? If so I think that's very reasonable behavior if someone tries to throw null at us somehow (presumably a coding error).
There was a problem hiding this comment.
Following up here, I think the implementation should support all possible cases:
- no data and no flag: Just ignore the hole packet. Could e.g. be possible in case we extend the Protobuf schema in a backwards compatible way in the future.
- data and no flag
- data and flag
- no data and flag
There was a problem hiding this comment.
@mxinden The switch and if seem to be independent to me, so the behavior you're describing is what I believe to already be the case.
For example, the FIN case in switch does a break not a return, so after handling that you head on to the if check for message
But if you got undefined/undefined you'd break out of the switch without doing anything and then pass over the if.
test/connection.browser.spec.ts
Outdated
| import first from 'it-first'; | ||
| import {fromString} from 'uint8arrays/from-string'; | ||
| import {v4} from 'uuid'; | ||
| // import { enable as enableLogger } from '@libp2p/logger'; |
test/connection.browser.spec.ts
Outdated
| break; | ||
| } | ||
| expect(responsed).to.be.true(); | ||
| }).timeout(54321); |
There was a problem hiding this comment.
Curious what the significance of this number is?
There was a problem hiding this comment.
It's larger than the default timeout. It may be a vestige of a time when I was trying to figure out why the connection is timing out. Will try removing it.
There was a problem hiding this comment.
Yeah, works fine.
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
mxinden/specs#1