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

libgit2: managed transport improvements #658

Merged
merged 7 commits into from
Apr 8, 2022
Merged

Conversation

pjbgf
Copy link
Member

@pjbgf pjbgf commented Apr 6, 2022

Stability and reliability improvements

  • Retry on stale connections: fixes the issue of long-running connections resulting in EOF for GitLab servers. Although this may also affect other Git Servers in different scenarios.
  • Handle closing of stale connections: dispose resources and remove connection from cache.
  • Optimise mutex on cached connections for decreased waiting time when competing go-routines try to create a SSH connection.

Low Impact Breaking Changes

  • HTTP(S) URLs must be 2048 characters long.
  • SSH Path must be 4096 characters long.

@pjbgf pjbgf added the area/git Git related issues and pull requests label Apr 6, 2022
@pjbgf pjbgf force-pushed the libgit2-fixes branch 5 times, most recently from 51bd024 to 4f88ceb Compare April 7, 2022 16:47
Paulo Gomes added 5 commits April 7, 2022 18:37
SSH servers that block the reuse of SSH connections for
multiple SSH sessions may lead to EOF when a new session
is being created.

This fixes the issue of long-running connections resulting
in EOF for GitLab servers.

Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
Internal and upstream calls to sshSmartSubtransport.Close()
when dealing with an stale connection, may lead to misleading
errors.

Focus should instead be redirected to ensuring that Close()
releases resources and ensures that a new SubTransport can be
created, so new operations can succeed.

Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
The major Git SaaS providers have repository URLs
for both HTTP and SSH that tops around 250
characters in length.

The limits chosen were a lot higher to align with use
cases in which users may have on-premise servers with
long domain names and paths.

For SSH the validation is around path length only,
which is now limited to 4096 characters, which is
at the higher end of the range in Linux.

For HTTP the validation is around the full URL
provided by the caller.

Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
Avoid asking for SSH credential in files, as they won't be
used. The cacheKeyAndConfig func already enforces this
behaviour.

Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
Cached connections can be shared across concurrent
operations, and their disposal must take that into
account to avoid closing a connection that is stale for
one goroutine, but is still valid for another.

Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
@pjbgf pjbgf marked this pull request as ready for review April 7, 2022 17:37
Paulo Gomes added 2 commits April 7, 2022 19:10
Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
Previously the mutex.Lock was acquired before creating
a new connection. The lock would then hold until the
process was finished, and all network latency would be
absorbed by other goroutines trying to establish a new
connection.

Now the lock is acquired after the connection has been
created. The downside of this approach is that concurrent
goroutine may be trying to open a connection to the same
target. The loser in the race will then have to Close the
connection and use the winner's instead.

Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/git Git related issues and pull requests
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants