Skip to content

Comments

Update the way read header is handled#80

Merged
pires merged 1 commit intopires:bugfix/reset_conn_read_deadline_after_successful_headerfrom
antoniomika:am/fix_read_deadline
Sep 8, 2021
Merged

Update the way read header is handled#80
pires merged 1 commit intopires:bugfix/reset_conn_read_deadline_after_successful_headerfrom
antoniomika:am/fix_read_deadline

Conversation

@antoniomika
Copy link
Contributor

@antoniomika antoniomika commented Aug 25, 2021 •
edited
Loading

edited by @pires

Fixes #75

@antoniomika antoniomika mentioned this pull request Aug 25, 2021
Merged
@pires
Copy link
Owner

pires commented Aug 26, 2021

I'll have to think about this and this week I'm already packed. Thanks @antoniomika

@pires pires self-requested a review August 26, 2021 10:01
@antoniomika antoniomika force-pushed the am/fix_read_deadline branch from d9ca515 to 13136de Compare August 27, 2021 17:49
@drakkan
Copy link
Contributor

drakkan commented Sep 6, 2021

Hi,

I just tested this patch with SFTPGo for proxy SSH and FTP connections, it looks fine. Are there any blockers to merge it? thanks

pires
pires reviewed Sep 7, 2021
View reviewed changes
Copy link
Owner

@pires pires left a comment

Choose a reason for hiding this comment

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

Can you please add comments to the code explaining what you're doing here? It took me a bit to understand why we'd care about keeping track of deadline times for read and write, and I'm not even sure I got it right. IIUC we must keep track so that ReadHeaderTimeout doesn't interfere w/ user provided deadlines.

@coveralls
Copy link

coveralls commented Sep 7, 2021 •
edited
Loading

Coverage Status

Coverage increased (+1.2%) to 95.543% when pulling 4a3af4f on antoniomika:am/fix_read_deadline into bab7dd2 on pires:bugfix/reset_conn_read_deadline_after_successful_header.

@antoniomika
Copy link
Contributor Author

antoniomika commented Sep 7, 2021

Hey @pires,

Added some more documentation, cleaned up some unneeded boilerplate and we should be ready to go!

Best,

pires
pires reviewed Sep 7, 2021
View reviewed changes
Copy link
Owner

@pires pires left a comment

Choose a reason for hiding this comment

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

This LGTM but I have two questions. Please, be patient w/ me here @antoniomika.

Thank you very, very much for this contribution!

@antoniomika
Copy link
Contributor Author

antoniomika commented Sep 7, 2021

@pires just a heads up, this should merge first, followed by #76 after this lands.

@pires
Copy link
Owner

pires commented Sep 8, 2021

@pires just a heads up, this should merge first, followed by #76 after this lands.

What is there in #76 that isn't solved here?

@drakkan
Copy link
Contributor

drakkan commented Sep 8, 2021

@pires just a heads up, this should merge first, followed by #76 after this lands.

What is there in #76 that isn't solved here?

To apply this patch you have to apply #76 first.

@drakkan drakkan mentioned this pull request Sep 8, 2021
Merged
pires
pires reviewed Sep 8, 2021
View reviewed changes
Copy link
Owner

@pires pires left a comment

Choose a reason for hiding this comment

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

Somehow #82 landed here and I'd like to give the chance for @drakkan to get their contribution in. Can you please remove this?

@pires
Copy link
Owner

pires commented Sep 8, 2021

@pires just a heads up, this should merge first, followed by #76 after this lands.

What is there in #76 that isn't solved here?

To apply this patch you have to apply #76 first.

Damn, I must have that espresso urgently! 😅

@pires pires self-assigned this Sep 8, 2021
@pires pires merged commit f19e669 into pires:bugfix/reset_conn_read_deadline_after_successful_header Sep 8, 2021
@antoniomika
Copy link
Contributor Author

antoniomika commented Sep 8, 2021

Somehow #82 landed here and I'd like to give the chance for @drakkan to get their contribution in. Can you please remove this?

Sorry about that, I should've checked if there was another PR with the static check fixes. I use code spaces and was a bit OCD with the linting issues. I actually started working on #73 but those changes were more complicated so I decided to leave them for when I had more time. You can remove those changes in your PR now and merge in #82.

@pires
Copy link
Owner

pires commented Sep 8, 2021

Yeah #73 is painful. Also have a branch for a long while now 😅

@antoniomika antoniomika deleted the am/fix_read_deadline branch November 10, 2021 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pires pires pires left review comments

+1 more reviewer

@drakkan drakkan drakkan left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@pires pires

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@antoniomika @pires @drakkan @coveralls