Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Implement the background mode in Windows. #23
feat: Implement the background mode in Windows. #23
Changes from all commits
150270a
ce19774
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Do we need to be creating a new pipe file for every request, or is there a way to reuse the same one? It seems weird to me to only be able to send a single request per pipe.
Also, you're not handling the
pipe_handle == None
case here.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.
For the reusing the NamedPipe instance, I will try to explain this. Short answer is there are not simple ways to do that. If we reuse the named pipe instance, this situation may happen, a
run
command is sent to the server followed by astop
command, the response for therun
command may be read by thestop
command. We are creating theNamedPipe instance
here under the sameNamedPipe server
. The relationship between aNamedPipe server
and aNamedPipe instance
can be likened to the relationship between anHTTP server
and anHTTP connection
. And in the HTTP/2 connection, they have a technique called multiplexing (multiple requests and responses can be sent concurrently over a single connection). NamedPipe doesn't support this (like HTTP/1).. we can implement this by ourselves if we observe performance issues.Good catch, I will handle it in next version.
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.
Similar to my comment in
FrontendRunner
, what do you think about applying the same thing here and using specialized classes for Windows vs Posix variants of theBackendRunner
?I think there's enough differences here to justify separate classes for each (e.g. Windows doesn't use signals, Windows uses Named Pipe Server vs HTTP server on Linux)
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.
It sounds like a good idea, and this is a optimization task so I prefer doing that outside of this PR, because this will touch the code used for Linux and we need to modify some tests as well. We have more than engineer working on the Adaptor runtime, so we can parallelize these tasks.
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.
Do we still need this
type: ignore
even though both servers implementshutdown()
?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.
yes.. because the definition is
self._server: Optional[Union[BackgroundHTTPServer, WinBackgroundNamedPipeServer]] = None
. It is possible that this isNone
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.
I see, we could add a
if self._server is not None:
here, I think we generally want to avoid usingtype: ignore
whenever possible so that our type checking is stronger.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.
I tried and got this error:
I think mypy cannot understand the inheritance, the
shutdown
for Linux is implemented in theBaseServer
. And the relationship isBaseServer -> TCPServer -> UnixStreamServer -> BackgroundHTTPServer
. In Windows,shutdown
is implemented in theWinBackgroundNamedPipeServer
directly.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.
Yeah seems like it, thanks for investigating!
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.
I think we can fix this by extracting the common functions of our server implementations to an interface. Right now, the
WinBackgroundNamedPipeServer
class implicitly implements a similar interface as ourBackgroundHTTPServer
class so that it's usable from here.I suggest we make an explicit common interface between the two and use that here instead. For now, it'll only need two methods:
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.
I think this won't work either...
BaseServer
is not defined by us it is a built-in class which is defined by python. If we insist on avoiding using the# type: ignore
here, I prefer doing this outside of this PR and already created a ticket for it.