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

Handle partially available data #24

Closed
rom1504 opened this issue Nov 5, 2015 · 16 comments
Closed

Handle partially available data #24

rom1504 opened this issue Nov 5, 2015 · 16 comments

Comments

@rom1504
Copy link
Member

rom1504 commented Nov 5, 2015

We used to return null in readers because the data might be partially available in <=1.6 minecraft version.
If we want to be able to handle these kind of behavior, we might want to throw a special exception in this kind of case, and then handle it properly in the parser. (I guess "wait" for more data and try again)

@roblabla
Copy link
Collaborator

This needs to be efficient, meaning we don't want to "lose" progress just because we are missing some data. This means we might want to go "continuation-style" (asynchronous read() function that returns a byte when it's available, etc).

@rom1504
Copy link
Member Author

rom1504 commented Nov 22, 2015

hmm, seems I do need this for classic.

@rom1504
Copy link
Member Author

rom1504 commented Nov 23, 2015

@rom1504
Copy link
Member Author

rom1504 commented Nov 23, 2015

What does that mean for readers that themselves call the read method (say containers for example) ? Do they need this to be the stream or something similar?

@roblabla
Copy link
Collaborator

Instead of a buffer, they'll be passed a stream probably?

On Mon, Nov 23, 2015, 2:10 PM Romain Beaumont notifications@github.com
wrote:

What does that mean for readers that themselves call the read method (say
containers for example) ? Do they need this to be the stream or something
similar?


Reply to this email directly or view it on GitHub
#24 (comment).

@rom1504
Copy link
Member Author

rom1504 commented Nov 23, 2015

Ah yeah that makes sense.
Here is the implementation of whatwg streams https://www.npmjs.com/package/whatwg-streams
It seems pretty experimental but if it works I'm fine with it.

@rom1504
Copy link
Member Author

rom1504 commented Nov 23, 2015

Hmm actually that thing on npm seems pretty old, we might need to depend on https://github.com/whatwg/streams/tree/master/reference-implementation

@roblabla
Copy link
Collaborator

Worse case scenario we can just depend on anything that exposes a similar
polling interface, it doesn't have to be whatwg streams (they are still in
heavy development!). I mostly posted those as an example of what I believed
the interface should look like ;)

On Mon, Nov 23, 2015, 2:32 PM Romain Beaumont notifications@github.com
wrote:

Hmm actually that thing on npm seems pretty old, we might need to depend
on https://github.com/whatwg/streams/tree/master/reference-implementation


Reply to this email directly or view it on GitHub
#24 (comment).

@rom1504
Copy link
Member Author

rom1504 commented Nov 23, 2015

Yeah but whatwg streams do seem reasonable, I'll try and see if they can be used.

@rom1504
Copy link
Member Author

rom1504 commented Nov 23, 2015

https://www.npmjs.com/package/pull-stream might be interesting

@rom1504
Copy link
Member Author

rom1504 commented Nov 23, 2015

These libs don't seem quite ready to be used (pull-stream works with callback and whatwg stream is unpublished).
I guess we would have to implement it ourselves ?

edit: maybe https://github.com/h13i32maru/eo.whatwg-streams

@rom1504
Copy link
Member Author

rom1504 commented Nov 23, 2015

@rom1504
Copy link
Member Author

rom1504 commented Dec 1, 2015

@roblabla what would that means for write/sizeOf ? no change to their API ?

rom1504 added a commit that referenced this issue Dec 1, 2015
@rom1504
Copy link
Member Author

rom1504 commented Dec 3, 2015

I'm starting to think it might be more efficient to keep the current sync model and just return null... (and retry to parse the packet when we got more data)
(or throw an exception instead of returning null)

rom1504 added a commit that referenced this issue Dec 3, 2015
rom1504 added a commit that referenced this issue Dec 4, 2015
rom1504 added a commit that referenced this issue Dec 4, 2015
@rom1504
Copy link
Member Author

rom1504 commented Feb 10, 2016

I might try again to make that promise thingy to work with nmp.

Maybe I will make an alternative PR with the "throw an error" option though. It sure is a little bit less efficient if lot of packets are cut, but in the general case it's actually very likely more efficient (no async overhead !) and it keeps the API sync which is nice.

@rom1504
Copy link
Member Author

rom1504 commented Feb 18, 2016

closing this
It's too inefficient to do it asyncly. We can always reopen later if needed

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

2 participants