Skip to content

[RFC] Simple IPC mechanism#717

Closed
jeffhostetler wants to merge 850 commits intogitgitgadget:jk/strvecfrom
jeffhostetler:simple-ipc
Closed

[RFC] Simple IPC mechanism#717
jeffhostetler wants to merge 850 commits intogitgitgadget:jk/strvecfrom
jeffhostetler:simple-ipc

Conversation

@jeffhostetler
Copy link

This series introduces a multi-threaded IPC mechanism called simple-ipc.
It allows a foreground Git process to communicate with an existing Git daemon
process using named pipes (on Windows) or unix domain sockets (on other
platforms).

This will serve as a basis for a future Git-aware FSMonitor service daemon
feature.

@dscho
Copy link
Member

dscho commented Sep 1, 2020

@jeffhostetler would you mind integrating dscho@889e38b and dscho@889e38b into your branch (git pull --ff-only https://github.com/dscho/git simple-ipc)? That should fix the build.

Copy link
Member

@dscho dscho left a comment

Choose a reason for hiding this comment

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

Phew. Lots to read, and I cannot even say that I (re-)read the backend code in detail...

unix-socket.c Outdated
* portion in `sockaddr_un.path`. This is problematic
* if called from threaded context.
*/
int size = strlen(path) + 1;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use size_t here?

Copy link
Member

Choose a reason for hiding this comment

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

Or inline strlen(path) + 1? Or avoid the arithmetic? Like so:

if (opts->disallow_chdir &&
    strlen(path) >= sizeof(sa.sun_path))
        return ENAMETOOLONG;

Copy link
Member

Choose a reason for hiding this comment

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

BTW there will also be the question, I am sure, why this is not folded into struct unix_sockaddr_context.

Copy link
Author

Choose a reason for hiding this comment

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

WRT the arithmetic, I was using the same spelling as in unix_sockaddr_init()
which does it that way.

Copy link
Author

Choose a reason for hiding this comment

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

WRT unix_sockaddr_context, that is passed uninitialized to unix_sockaddr_init() and populated by it, if a CD was necessary.
So changing the initialization details felt wrong. However, _context is
private and all the callers are static, so maybe it wouldn't be so bad.

Let me take a look at this.

Copy link
Author

Choose a reason for hiding this comment

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

I overhauled the disallow-chdir commit. Let me know if this looks better.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this looks very good!

Copy link
Member

Choose a reason for hiding this comment

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

I would still recommend to put the if (ctx->disallow_chdir) { errno = ENAMETOOLONG; return -1; } inside the existing if (size > sizeof(sa->sun_path)) { ... } block, but that's a minor thing.

Copy link
Author

Choose a reason for hiding this comment

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

i thought about that but it means I have to touch the code inside the next block -- they init a variable as they declare it and it looks like an expensive function call, and I'd have to undo that to get my new if statement wedged in there. and that seems a bit too much. but i'm open to suggestion here.

return 0;
}

/* A randomly chosen value. */
Copy link
Member

Choose a reason for hiding this comment

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

trace2_data_string("ipc-server", NULL, "try-listen-force", path);

opts.force_unlink_before_bind = 1;
fd_listen = unix_stream_listen_gently(path, &opts);
Copy link
Member

Choose a reason for hiding this comment

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

There is a race here, right? Between unix_stream_connect() and unix_stream_listen_gently(), another service process could have just started listening. Maybe clarify that in an inline comment? (To me, the comment above the function does not really clarify this point.)

Copy link
Author

Choose a reason for hiding this comment

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

i've completely reworded the comments above and in-line in that function. let me know if that helps.

@jeffhostetler jeffhostetler force-pushed the simple-ipc branch 2 times, most recently from ae3b033 to 6624f47 Compare September 1, 2020 16:57
@jeffhostetler
Copy link
Author

I just saw your note about about fixing the CI builds and put those 2 commits on top for now. Thanks!!!

@jeffhostetler jeffhostetler force-pushed the simple-ipc branch 6 times, most recently from 6f3929c to eef62bc Compare September 1, 2020 21:37
@jeffhostetler
Copy link
Author

@dscho Hey, big thanks for major review and suggestions. I think I have addressed most all of them.

@gitgitgadget
Copy link

gitgitgadget bot commented Sep 2, 2020

There is an issue in commit 7b9c519:
Commit checks stopped - the message is too short

@jeffhostetler jeffhostetler changed the base branch from master to jk/strvec September 16, 2020 20:20
@jeffhostetler
Copy link
Author

Rebased my simple-ipc branch from "git.git master v2.28 + 11th batch" back to "v2.28 + jk/strvec".

gitster and others added 9 commits September 18, 2020 17:58
"git worktree add" learns that the "-d" is a synonym to "--detach"
option to create a new worktree without being on a branch.

* es/wt-add-detach:
  git-worktree.txt: discuss branch-based vs. throwaway worktrees
  worktree: teach `add` to recognize -d as shorthand for --detach
  git-checkout.txt: document -d short option for --detach
Optimization around submodule handling.

* os/collect-changed-submodules-optim:
  submodule: suppress checking for file name and ref ambiguity for object ids
"git status --short" quoted a path with SP in it when tracked, but
not those that are untracked, ignored or unmerged.  They are all
shown quoted consistently.

* jc/quote-path-cleanup:
  quote: turn 'nodq' parameter into a set of flags
  quote: rename misnamed sq_lookup[] to cq_lookup[]
  wt-status: consistently quote paths in "status --short" output
  quote_path: code clarification
  quote_path: optionally allow quoting a path with SP in it
  quote_path: give flags parameter to quote_path()
  quote_path: rename quote_path_relative() to quote_path()
"git diff/show" on a change that involves a submodule used to read
the information on commits in the submodule from a wrong repository
and gave a wrong information when the commit-graph is involved.

* mf/submodule-summary-with-correct-repository:
  submodule: use submodule repository when preparing summary
  revision: use repository from rev_info when parsing commits
Allow maintainers to tweak $(TAR) invocations done while making
distribution tarballs.

* jc/dist-tarball-tweak:
  Makefile: allow extra tweaking of distribution tarball
Unlike "git config --local", "git config --worktree" did not fail
early and cleanly when started outside a git repository.

* mt/config-fail-nongit-early:
  config: complain about --worktree outside of a git repo
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Call hashwrite_be32() instead of open-coding it.  This shortens the code
a bit and makes it easier to read.

Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
git-p4 unshelve uses HEAD^$n to find the parent commit, which
fails if there is an additional commit.

Signed-off-by: Luke Diamand <luke@diamand.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
jiangxin and others added 24 commits October 12, 2020 15:19
* 'master' of github.com:alshopov/git-po:
  l10n: bg.po: Updated Bulgarian translation (5013t)
I am excited. Because I like a lot languages, and because I believe this
is the way to contribute to a large number of Portuguese speaking
person.

Jiang Xin and last Portuguese team gave me the lead. Thank you very
much. Honored to be a part of such a project.

Signed-off-by: Daniel Santos <hello@brighterdan.com>
Pull from the language Coordenator repository and
`make` done at the top-level directory.

Signed-off-by: Daniel Santos <hello@brighterdan.com>
Signed-off-by: Tran Ngoc Quan <vnwildman@gmail.com>
* 'pt-PT' of github.com:git-l10n-pt-PT/git-po:
  l10n: pt_PT: make on po/pt_PT.po
  l10n: Portuguese translation team has changed. Wohoo!
Reviewed-by: Ralf Thielow <ralf.thielow@gmail.com>
Reviewed-by: Phillip Szelat <phillip.szelat@gmail.com>
Signed-off-by: Matthias Rüster <matthias.ruester@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
On Mac, tooltips are not automatically removed when a window loses
focus. Furthermore, mouse-move events are only dispatched to the active
window, which means that if we Command-tab to another application while
a tool tip is showing, the tool tip will stay there forever (in front of
other applications). So we must hide it manually when we lose focus.

Do this unconditionally here (i.e. without if {[is_MacOSX]}); it
shouldn't hurt on other platforms, even though they don't seem to have
this problem.

Signed-off-by: Stefan Haller <stefan@haller-berlin.de>
Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>
Make sure `git gui blame` tooltips are destroyed once the window loses
focus on MacOS.

* sh/blame-tooltip:
  git-gui: blame: prevent tool tips from sticking around after Command-Tab
* https://github.com/prati0100/git-gui:
  git-gui: blame: prevent tool tips from sticking around after Command-Tab
  git-gui: improve dark mode support
  git-gui: fix mixed tabs and spaces; prefer tabs
Translate 124 new messages (5013t0f0u) for git 2.29.0.

Reviewed-by: 依云 <lilydjwg@gmail.com>
Reviewed-by: Fangyi Zhou <me@fangyi.io>
Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
* 'master' of github.com:Softcatala/git-po:
  l10n: Update Catalan translation
l10n for Git 2.29.0 round 2

* tag 'l10n-2.29.0-rnd2' of git://github.com/git-l10n/git-po:
  l10n: zh_CN: for git v2.29.0 l10n round 1 and 2
  l10n: de.po: Update German translation for Git 2.29.0
  l10n: vi(5013t): Updated translation for v2.29.0 rd2
  l10n: pt_PT: make on po/pt_PT.po
  l10n: Portuguese translation team has changed. Wohoo!
  l10n: bg.po: Updated Bulgarian translation (5013t)
  l10n: sv.po: Update Swedish translation (5013t0f0u)
  l10n: it.po: update the Italian translation
  l10n: tr: v2.29.0 round 2
  l10n: zh_TW.po: v2.29.0 round 2 (2 untranslated)
  l10n: fr: v2.29.0 rnd 2
  l10n: git.pot: v2.29.0 round 2 (1 new, 1 removed)
  l10n: fr: v2.29.0 rnd 1
  l10n: it.po: update the Italian translation for Git 2.29.0 round 1
  l10n: tr: v2.29.0 round 1
  l10n: Update Catalan translation
  l10n: git.pot: v2.29.0 round 1 (124 new, 42 removed)
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Teach packet_write_gently() to use a stack buffer rather than a static
buffer when composing the packet line message.  This helps get us ready
for threaded operations.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
So far, the (possibly indirect) callers of `get_packet_data()` can ask
that function to return an error instead of `die()`ing upon end-of-file.
However, random read errors will still cause the process to die.

So let's introduce an explicit option to tell the packet reader
machinery to please be nice and only return an error.

This change prepares pkt-line for use by long-running daemon processes.
Such processes should be able to serve multiple concurrent clients and
and survive random IO errors.  If there is an error on one connection,
a daemon should be able to drop that connection and continue serving
existing and future connections.

This ability will be used by a Git-aware "Internal FSMonitor" feature
in a later patch series.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
…uf()

This function currently has only one caller: `apply_multi_file_filter()`
in `convert.c`. That caller wants a flush packet to be written after
writing the payload.

However, we are about to introduce a user that wants to write many
packets before a final flush packet, so let's extend this function to
prepare for that scenario.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The `read_packetized_to_strbuf()` function reads packets into a strbuf
until a flush packet has been received. So far, it has only one caller:
`apply_multi_file_filter()` in `convert.c`. This caller really only
needs the `PACKET_READ_GENTLE_ON_EOF` option to be passed to
`packet_read()` (which makes sense in the scenario where packets should
be read until a flush packet is received).

We are about to introduce a caller that wants to pass other options
through to `packet_read()`, so let's extend the function signature
accordingly.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Brief design documentation for new IPC mechanism allowing
foreground Git client to talk with an existing daemon process
at a known location using a named pipe or unix domain socket.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Create Windows implementation of "simple-ipc" using named pipes.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Create a gentle version of `unix_stream_listen()`.  This version does
not call `die()` if a socket-fd cannot be created and does not assume
that it is safe to `unlink()` an existing socket-inode.

`unix_stream_listen()` uses `unix_stream_socket()` helper function to
create the socket-fd.  Avoid that helper because it calls `die()` on
errors.

`unix_stream_listen()` always tries to `unlink()` the socket-path before
calling `bind()`.  If there is an existing server/daemon already bound
and listening on that socket-path, our `unlink()` would have the effect
of disassociating the existing server's bound-socket-fd from the socket-path
without notifying the existing server.  The existing server could continue
to service existing connections (accepted-socket-fd's), but would not
receive any futher new connections (since clients rendezvous via the
socket-path).  The existing server would effectively be offline but yet
appear to be active.

Furthermore, `unix_stream_listen()` creates an opportunity for a brief
race condition for connecting clients if they try to connect in the
interval between the forced `unlink()` and the subsequent `bind()` (which
recreates the socket-path that is bound to a new socket-fd in the current
process).

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Calls to `chdir()` are dangerous in a multi-threaded context.  If
`unix_stream_listen()` is given a socket pathname that is too big to
fit in a `sockaddr_un` structure, it will `chdir()` to the parent
directory of the requested socket pathname, create the socket using a
relative pathname, and then `chdir()` back.  This is not thread-safe.

Add `disallow_chdir` flag to `struct unix_sockaddr_context` and change
all callers to pass an initialized context structure.

Teach `unix_sockaddr_init()` to not allow calls to `chdir()` when flag
is set.

Extend the public interface to `unix_stream_listen_gently()` to also
expose this new flag.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Create unit tests for "simple-ipc".  These are currently only enabled
on Windows.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Create Unix domain socket based implementation of "simple-ipc".

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
@gitgitgadget
Copy link

gitgitgadget bot commented Oct 20, 2020

The pull request has 850 commits. The max allowed is 30. Please split the patch series into multiple pull requests. Also consider squashing related commits.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 20, 2020

There are merge commits in this Pull Request:

45f462b5c5a55039c35fc7001f2256329220bf4f
2ce9d4e5c7186e15f2e2f7be06a9db52b5e61bde
9d4e7ec4d9e695d95cc7474f59fba8cc38580e29
4831c23f75556df8906a508454d70faca0facd27
52bcf6e181eabe55266e6c8e85cfaf336a013f3d
80cacaec41b0fde4900ffef5f4374a9365db0755

Please rebase the branch and force-push.

@jeffhostetler
Copy link
Author

Rebased simple-ipc onto v2.29.0 in my branch.
I'm going to close this PR and open a new one with v2.29.0 as the base commit.

@jeffhostetler
Copy link
Author

See #766

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.