Skip to content

net/textproto: dont't ignore error in skipSpace function #54281

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

Closed
wants to merge 3 commits into from

Conversation

cz2h
Copy link

@cz2h cz2h commented Aug 5, 2022

skipSpace ignores the error it encounters while using a reader
which seems like a bug. Refactor the function and have it
returns the errors.

Modified readContinuedLineSlice and have it ignore io.EOF
while something is ever read in the function to keep its
behaviour unchanged.

Update #53858

@gopherbot
Copy link
Contributor

This PR (HEAD: c724099) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/421554 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 1:

(3 comments)


Please don’t reply on this GitHub thread. Visit golang.org/cl/421554.
After addressing review feedback, remember to publish your drafts!

@cz2h cz2h changed the title net/textproto: refactor return type of skipSpace function net/textproto: dont't ignore error in skipSpace function Aug 6, 2022
@gopherbot
Copy link
Contributor

Message from chuhao zeng:

Patch Set 2:

(3 comments)


Please don’t reply on this GitHub thread. Visit golang.org/cl/421554.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 2:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/421554.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 19c461c) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/421554 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from chuhao zeng:

Patch Set 3:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/421554.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 3:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/421554.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from chuhao zeng:

Patch Set 3:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/421554.
After addressing review feedback, remember to publish your drafts!

@cz2h cz2h closed this Aug 17, 2022
@kayrus
Copy link

kayrus commented Aug 17, 2022

@zengchu2 why this PR was closed?

@cz2h
Copy link
Author

cz2h commented Aug 17, 2022

@zengchu2 why this PR was closed?

It seems that changes to skipSpace will affect the behaviour of readContinuedLine, making it return a EOF after reading the last line of text. On the other hand, if we somehow ignore this EOF if we are returning something, then this change seems not that necessary cause it cannot fix corner case mentioned in the issue.

Probably there is another way of fixing that issue?

@kayrus
Copy link

kayrus commented Aug 17, 2022

Probably there is another way of fixing that issue?

not sure, but silently swallowing the err is also not good :\

@cz2h
Copy link
Author

cz2h commented Aug 17, 2022

Agree with that. But I also found it little bit hard to provide a proper example test case for this refactoring.

For regular io readers, the error can be detected during the next time when readContinuedLine is called. I guess this is why the error is ignored in skipSpace

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.

3 participants