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

load: ignore context cancel error #64

Merged
merged 2 commits into from Dec 7, 2022
Merged

load: ignore context cancel error #64

merged 2 commits into from Dec 7, 2022

Conversation

asjdf
Copy link
Contributor

@asjdf asjdf commented Nov 30, 2022

Describe the pull request

I recently noticed the error session: load: read: get: context canceled in my sentry panel.

I traced the error into this package and found that it will panic when the user closes the connection.

Actually, there are some fixes applied in line 188 but it is not enough.

Because the context canceled before the next handlers, I use c.ResponseWriter().WriteHeader(0) to end this handler chain.

By the way, I am looking forward flamego support the method like Abort() in gin to prevents pending handlers from being called.

Link to the issue: n/a

Checklist

  • I agree to follow the Code of Conduct by submitting this pull request.
  • I have read and acknowledged the Contributing guide.
  • I have added test cases to cover the new code.

@asjdf
Copy link
Contributor Author

asjdf commented Dec 3, 2022

@unknwon sorry to bother you, but do you have time to review this pr?

@wuhan005 wuhan005 changed the title fix: fix panic when context.Canceled load: ignore context cancel error Dec 4, 2022
session.go Outdated Show resolved Hide resolved
@unknwon
Copy link
Member

unknwon commented Dec 6, 2022

Apologies for the delay, will take a look today!

@unknwon
Copy link
Member

unknwon commented Dec 7, 2022

By the way, I am looking forward flamego support the method like Abort() in gin to prevents pending handlers from being called.

If I understand your ask correctly, you may want to look at https://flamego.dev/core-services.html#response-stream for how to archive that.

TL;DR as soon as a status code is being written, subsequent handlers won't be called.

@asjdf
Copy link
Contributor Author

asjdf commented Dec 7, 2022

顺便说一下,我期待 flamego 支持像Abort()gin 中的方法,以防止挂起的处理程序被调用。

如果我正确理解您的问题,您可能需要查看https://flamego.dev/core-services.html#response-stream了解如何存档。

TL;DR 一旦写入状态代码,就不会调用后续处理程序。

I know about this feature, and that's why I added c.ResponseWriter().WriteHeader(0) to the code.

@asjdf
Copy link
Contributor Author

asjdf commented Dec 7, 2022

I think there should be some problem with my modification, if you can explain the correct workflow, I can make a modification.

session.go Outdated Show resolved Hide resolved
@unknwon
Copy link
Member

unknwon commented Dec 7, 2022

I just committed a suggestion, I think things should work as expected now.

Copy link
Member

@unknwon unknwon left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@unknwon
Copy link
Member

unknwon commented Dec 7, 2022

I'm gonna merge and release a new patch version. If you have any other suggestions that are not included, we can always work on a followup PR, since this one is purely beneficial.

@unknwon unknwon merged commit f08e1fc into flamego:main Dec 7, 2022
@unknwon
Copy link
Member

unknwon commented Dec 7, 2022

https://github.com/flamego/session/releases/tag/v1.2.2 has been created for this merge.

@asjdf
Copy link
Contributor Author

asjdf commented Dec 7, 2022

Thanks for reviewing.
This blog seems to be misleading me https://neojos.com/blog/2018/03-17-http%E6%80%BB%E7%BB%93-%E7%8A%B6%E6%80%81%E7%A0%81/#0%E8%B6%85%E6%97%B6%E5%AE%A2%E6%88%B7%E7%AB%AF%E4%B8%BB%E5%8A%A8%E6%96%AD%E5%BC%80%E8%BF%9E%E6%8E%A5.

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