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

Mark Windows npipe support as stable (with a fix) #865

Merged
merged 15 commits into from
Sep 13, 2018
Merged

Conversation

bsideup
Copy link
Member

@bsideup bsideup commented Sep 12, 2018

There was a bug in FramedResponseStreamHandler: if buffer has two lines, only first one was processed. Fixed by manually chunking the buffer

Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

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

Some small questions and I think we are good to go then, awesome! We can also update the docs accordingly.

@@ -36,7 +36,9 @@ shadowJar {
'META-INF/services/org.jvnet.hk2.external.generator.*',
'META-INF/services/org.glassfish.jersey.internal.spi.*',
'META-INF/services/java.security.Provider',
'META-INF/services/java.sql.Driver',
Copy link
Member

Choose a reason for hiding this comment

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

Oh, I think we can remove this now also?

return os;
}

public void setTcpNoDelay(boolean bool) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need non @Override dummy methods?

handler.channelRead(null, Unpooled.wrappedBuffer(buffer, 0, bytesReceived));
int offset = 0;
for (int i = 0; i < bytesReceived; i++) {
if (buffer[i] == '\n' || i == bytesReceived - 1) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's just extract those two boolean conditions into two variables, lineBreak and endOfBuffer. Maybe even adding a small comment that line break can happen, if the daemon is answering very fast and it means we got multiple responses.

int offset = 0;
for (int i = 0; i < bytesReceived; i++) {
// some handlers like JsonResponseCallbackHandler do not work with multi-line buffers
boolean isLineBreak = buffer[i] == '\n';
Copy link
Contributor

Choose a reason for hiding this comment

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

This one matches any line break, but I assume only line breaks between json objects should be considered (as delimiter for line-delimited JSON). So, what happens when something like below is part of the response? 🤔

{"foo":"bar\nbaz"}
{"anything":"else"}

Copy link
Member Author

@bsideup bsideup Sep 13, 2018

Choose a reason for hiding this comment

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

\\n != \n :)

Copy link
Contributor

Choose a reason for hiding this comment

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

oh my. gotcha! :)

@bsideup bsideup merged commit 52d7126 into master Sep 13, 2018
@bsideup bsideup deleted the windows_fixes branch September 13, 2018 14:05
@aulea aulea mentioned this pull request Sep 14, 2018
rnorth pushed a commit that referenced this pull request Sep 20, 2018
#756 tested on Windows. 
Fixed RegistryAuthLocatorTest on Windows and also allowed better fallbacks from running credential provider (to allow lookup alternative AuthConfigs), when:
1) there is no hostName, then there is no point to ask credentials
2) when credential helper response with "credentials not found in native keychain" to try other resources

Main reason for failing for me on Windows machine was #710 changes. When i used Netty or OkHttp together with npipe, then it worked fine. Yesterday evening i found out the reason and today morning i found also fix in master for that :-) - #865, breaking docker response by line breaks.
@rnorth
Copy link
Member

rnorth commented Sep 21, 2018

Released for preview in 1.9.0-rc2, to be published on Bintray.

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

Successfully merging this pull request may close these issues.

None yet

4 participants