Skip to content
New issue

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

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

Already on GitHub? # to your account

ByteBuffer docs issue: Does not dissuade users from using get* #2469

Closed
weissi opened this issue Jul 12, 2023 · 3 comments · Fixed by #2499
Closed

ByteBuffer docs issue: Does not dissuade users from using get* #2469

weissi opened this issue Jul 12, 2023 · 3 comments · Fixed by #2499

Comments

@weissi
Copy link
Member

weissi commented Jul 12, 2023

ByteBuffer.get* is very often abused/misused. For example, what I see a lot is buf.getData(at: 0, length: buf.readableBytes)!.

Obviously the user should do Data(buffer: buf) here because it's nicer and shorter.

But most importantly, the at: 0 is actually wrong. We should very much dissuade users from doing get*. They should do TargetType(buffer: buf) or read*. The docs don't say that.

@natikgadzhi
Copy link
Contributor

Uh oh. So I was working on the hexdump helpers (#2475), and very much used getInteger(at: and getString(at:. While I access bytes at specific indices and don't just do at: 0, can you point me at why using 0 as index is wrong?

@weissi
Copy link
Member Author

weissi commented Jul 14, 2023

Uh oh. So I was working on the hexdump helpers (#2475), and very much used getInteger(at: and getString(at:. While I access bytes at specific indices and don't just do at: 0, can you point me at why using 0 as index is wrong?

The get* methods are very low-level and most of the time not the right tool (can be dangerous but not in an undefined behaviour kinda way fortunately). The readable bytes of a ByteBuffer start are readerIndex ..< writerIndex (see doc comments). So 0 might be before readerIndex so you can't use that. For users we actually very much recommend not to use get* at all unless you have a special network protocol that talks in byte positions (and you'd need to make sure that your readerIndex is 0). See also #2468

So if you wanted to loop over all the bytes, then you could either do

for index in buffer.readerIndex ..< buffer.writerIndex {
    print(buffer.getInteger(at: index, as: UInt8.self))
}

or via the Collection conformance

for byte in buffer.readableBytesView {
    print(byte)
}

The fastest way to access the bytes wholesale is through unsafe land buffer.withUnsafeReadableBytes { bytesPtr in ... }.


Your hex dumping stuff will be part of NIO itself so for you the get* methods might be the right choice but be sure to only look inside of readerIndex ..< writerIndex.

@natikgadzhi
Copy link
Contributor

As the hex dump PR is close to a wrap, I think I stumbled at this enough times to try and put up a PR. I'll start with #2468, that one seems to be smaller.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants