Skip to content
This repository has been archived by the owner on Apr 19, 2023. It is now read-only.

Windows Named Pipes: Allow reading fewer bytes #109

Merged
merged 2 commits into from Dec 4, 2017

Conversation

eed3si9n
Copy link
Contributor

Ref #92

read(b, off, len) can sometimes be called when the length is unknown. This changes so that it uses the number of available bytes returned from GetOverlappedResult function, copies the bytes, and returns it.

Ref facebookarchive#92

`read(b, off, len)` can sometimes be called when the length is unknown. This changes so that it uses the number of available bytes returned from `GetOverlappedResult` function, copies the bytes, and returns it.
@@ -117,17 +117,15 @@ public int read(byte[] b, int off, int len) throws IOException {
}
}

IntByReference read = new IntByReference();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you rename read to r? Can you name it properly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you rename read to r?

I thought it's confusing to name a local variable as read (past perfect tense?) inside the implementation of a method named read. Given that this reference is a throw-away that later gets used as int actualLen, I though a single letter variable name is most proper here.

Can you name it properly?

I did what I thought was good. I'd be happy to change to something else if you have suggestions.

@ilya-klyuchnikov
Copy link
Contributor

ilya-klyuchnikov commented Nov 29, 2017

read(b, off, len) can sometimes be called when the length is unknown.

When?

Nailgun protocol is designed in such a way that len is always known. If we read less than len, it indicates a problem with nailgun connection.

@eed3si9n
Copy link
Contributor Author

When?

Nailgun protocol is designed in such a way that len is always known. If we read less than len, it indicates a problem with nailgun connection.

I saw it break when I was using this code in sbt server, which has code like this:

bytesRead = in.read(readBuffer)

@ilya-klyuchnikov
Copy link
Contributor

I see. Could you then make this logic configurable - something like an extra parameter/field to throw or not throw an exception when less bytes than requested have been read? In the context of nailgun (because of some internals of the protocol) such situation is a signal that the connection

@ilya-klyuchnikov
Copy link
Contributor

... the connection is unhealthy. While in your context it can be ok. I would like to keep this logic for nail gun.

This adds requireStrictLength parameter to NGWin32NamedPipeSocket and NGWin32NamedPipeServerSocket so the socket can return an inputstream whose `read(byte[], int, int)` requires exact length, as opposed to passing in a buffer array.
@eed3si9n
Copy link
Contributor Author

eed3si9n commented Dec 1, 2017

@ilya-klyuchnikov This feels odd to me since most read(...) I've seen allows reading fewer bytes, but I did add a parameter called requireStrictLength to restore the original behavior.

@ilya-klyuchnikov ilya-klyuchnikov merged commit e3416a6 into facebookarchive:master Dec 4, 2017
@ilya-klyuchnikov
Copy link
Contributor

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants