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

Pass the Request to settingsOnException handlers when available. #326

Merged
merged 1 commit into from Jan 14, 2015

Conversation

Projects
None yet
5 participants
@romanb
Copy link
Contributor

romanb commented Jan 13, 2015

This PR tries to address what looks like a regression. At one point the Maybe Request argument was added to the settingsOnException handler and the Request passed along [1]. The related ticket was #173. Only a couple of commits later the relevant line of code got lost [2]. As a result, it seems as if the Maybe Request argument is actually never passed a request, which is unfortunate.

[1] b2fa01e#diff-39726960319491135a952279b66943f2R249
[2] 67a0bf5#diff-39726960319491135a952279b66943f2L245

@gregwebs

This comment has been minimized.

Copy link
Member

gregwebs commented Jan 13, 2015

So the second commit could be a bad merge.

snoyberg added a commit that referenced this pull request Jan 14, 2015

Merge pull request #326 from romanb/bugfix/missing-request-on-exception
Pass the Request to settingsOnException handlers when available.

@snoyberg snoyberg merged commit 9642039 into yesodweb:master Jan 14, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@snoyberg

This comment has been minimized.

Copy link
Member

snoyberg commented Jan 14, 2015

Good catch, thank you!

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

kazu-yamamoto commented Jan 23, 2015

This patch introduced a huge space leak because this code is not tail recursive. I'm now working to solve this.

@fumieval

This comment has been minimized.

Copy link

fumieval commented Jan 23, 2015

The cause is that recvSendLoop and processRequest are mutually recursive, while recvSendLoop piles up the user-supplied handler repeatedly.

Does just putting them into L281 (

recvSendLoop addr istatus src `E.catch` \e -> do
) make sense?

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

kazu-yamamoto commented Jan 23, 2015

@fumieval Yeah. What's exactly I'm wondering.

@romanb What was your intention to introduce one more catch?

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

kazu-yamamoto commented Jan 23, 2015

OK. req can be only used there. I understand.

kazu-yamamoto added a commit that referenced this pull request Jan 23, 2015

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

kazu-yamamoto commented Jan 23, 2015

The patch above solves the space leak. It's not elegant. If you guys have any ideas to make code more elegant, please let me know. Anyway, I have released a new version of Warp to save the world.

@snoyberg

This comment has been minimized.

Copy link
Member

snoyberg commented Jan 23, 2015

Thanks Kazu

On Thu, Jan 22, 2015, 10:53 PM Kazu Yamamoto notifications@github.com
wrote:

The patch above solves the space leak. It's not elegant. If you guys have
any ideas to make code more elegant, please let me know. Anyway, I have
released a new version of Warp to save the world.


Reply to this email directly or view it on GitHub
#326 (comment).

@romanb

This comment has been minimized.

Copy link
Contributor Author

romanb commented Jan 23, 2015

@kazu-yamamoto Apologies for overlooking this important detail and thanks for fixing it!

@kazu-yamamoto kazu-yamamoto referenced this pull request Feb 4, 2015

Closed

Memory leaks in warp #318

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.