-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
afe7e7b
to
728a27f
Compare
There was a problem hiding this 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.
core/build.gradle
Outdated
@@ -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', |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
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"}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\\n
!= \n
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh my. gotcha! :)
#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.
Released for preview in 1.9.0-rc2, to be published on Bintray. |
There was a bug in
FramedResponseStreamHandler
: if buffer has two lines, only first one was processed. Fixed by manually chunking the buffer