Skip to content

[2.x] fix: always end transaction when socket is closed prematurely (#1439) #1445

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

Merged
merged 1 commit into from
Oct 16, 2019

Conversation

watson
Copy link
Contributor

@watson watson commented Oct 9, 2019

Backports the following commits to 2.x:

…c#1439)

This fixes a race condition that in some cases meant that a request
transaction was never ended if the underlying TCP socket was closed by
the client and the server didn't discover this in time.

Fixes elastic#1411
@watson watson added the backport label Oct 9, 2019
@Qard
Copy link
Contributor

Qard commented Oct 9, 2019

Failing on commit message length, apparently. Not sure why the original PR didn't catch that. 🤔

@Qard
Copy link
Contributor

Qard commented Oct 9, 2019

Oh. because of the extra PR reference at the end. 😅

@Qard
Copy link
Contributor

Qard commented Oct 9, 2019

How should we deal with that? We already leave space for the PR reference at the end of the original commit, but it doesn't take into account the doubling from backporting. 🤔

@watson
Copy link
Contributor Author

watson commented Oct 9, 2019

Yeah, it's a bit unfortunate. I think it would be best, in the long run, to move the PR id to the body just like we do in Node core. In the past, I've just resorted to manually editing the commit merge when squash-merging

@Qard
Copy link
Contributor

Qard commented Oct 10, 2019

Maybe we could add something to the backport tool for filtering commit messages somehow? The commit reference is a standard format on GitHub, so it makes sense to have a config for backport to detect and remove it. The reference to the origin PR already exists in the backport PR anyway. 🤔

@watson
Copy link
Contributor Author

watson commented Oct 11, 2019

Could be (ping @sqren). But for now, I think it's ok to just manually edit the commit merge message 👍

@sorenlouv
Copy link
Member

There are options to set the patterns for PR title and PR description but it's pretty simplistic. Currently no way to format the commit message automatically.

https://github.com/sqren/backport/blob/master/docs/configuration.md#prtitle

@Qard Qard merged commit da5cd2c into elastic:2.x Oct 16, 2019
@Qard
Copy link
Contributor

Qard commented Oct 16, 2019

Landed with a manual commit title change in the squash so it'd pass the lint. We can figure out how to deal with the general backport commit length issue later.

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

Successfully merging this pull request may close these issues.

3 participants