Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an option to the parser to disable partial packets #57

Closed
rom1504 opened this issue Feb 27, 2016 · 13 comments
Closed

Add an option to the parser to disable partial packets #57

rom1504 opened this issue Feb 27, 2016 · 13 comments

Comments

@rom1504
Copy link
Member

rom1504 commented Feb 27, 2016

Useful for protocol that have length prefixed packets.
The benefit of disabling it is mainly to produce an error if the amount of bytes read is different from the size of the buffer.

@rom1504
Copy link
Member Author

rom1504 commented Feb 27, 2016

An option or a separate Parser, depending on what's easier to implement.

@roblabla
Copy link
Collaborator

I wonder if a user-defined datatype would do the trick ?

{
  type: "fixed",
  typeArgs: {
    size: "selector",
    contents: dataType
  }
}

(A note on selector : that's #35)

@rom1504
Copy link
Member Author

rom1504 commented Feb 28, 2016

Well it's more of a Parser stream thing. It's kind of meta, like PrismarineJS/minecraft-data#115

We don't store the length prefixed packet info thing in the .json (yet ?)

@rom1504
Copy link
Member Author

rom1504 commented Feb 29, 2016

(will avoid error like Error: Deserialization error for play.toClient : Read error for name : 3365 is not in the mappings value in nmp when the protocol is wrong, it will instead say something about the length of the packet)

@rom1504
Copy link
Member Author

rom1504 commented Feb 29, 2016

Ah I see your point. Indeed something like that would work, but only if we handle the whole packet within protodef.

That might be possible but it requires us to handle both compression and framing within protodef. But compression is async so we can't quite do that with the current interface ( #49 )

Something in between might be to make the framer and compression streams keep the length so the parser could use it.

@rom1504
Copy link
Member Author

rom1504 commented Feb 29, 2016

Ah actually I think it might be possible to do that by passing the size of this.queue (it shouldn't contain too much stuff since we will error out and empty it in case of pb) as a typeArgs when reading a packet in the serializer https://github.com/roblabla/ProtoDef/blob/master/src/serializer.js#L44

@rom1504
Copy link
Member Author

rom1504 commented Feb 29, 2016

First thing to do : create an example test case (in examples/) that requires this "fixed" to error out on the right packet.

rom1504 added a commit to rom1504/node-ProtoDef that referenced this issue Feb 29, 2016
@roblabla
Copy link
Collaborator

roblabla commented Mar 1, 2016

OK, so concerning this. We don't need ProtoDef to actually handle compression and stuff. We just need a way to pass variables to ProtoDef. This facility actually already exists with the rootNode : just pass some data there, and it will be accessible.

I guess you'll need something more powerful than the standard "Serializer" though, as Serializer doesn't have any way to pass data like that.

@rom1504
Copy link
Member Author

rom1504 commented Mar 1, 2016

it's totally possible to pass the size of the chunk as a typeArgs when reading a packet. But that assumes the chunks are already split which bring us back to #60 (comment)

(this issue only concern the Parser, not the Serializer)

@rom1504
Copy link
Member Author

rom1504 commented Apr 2, 2016

the "fixed" type would indeed make sense. We have something similar in raknet and it would help.

@rom1504
Copy link
Member Author

rom1504 commented Jan 1, 2018

this is important. Because of this we have this kind PrismarineJS/node-minecraft-protocol#522 of silly errors.

I think we should just provide a second parser for now.

@Saiv46
Copy link
Contributor

Saiv46 commented Jun 13, 2020

Don't we have Parser and FullPacketParser already? Also, that's not a ProtoDef should care of.

@rom1504
Copy link
Member Author

rom1504 commented Jun 13, 2020

Yeah this is fixed, and definitely what protodef takes care of

@rom1504 rom1504 closed this as completed Jun 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants