Skip to content

fix: app hangs on starlette when sufficiently many cookies are set#751

Merged
maartenbreddels merged 2 commits intomasterfrom
08-26-ci_automatic_testing_suite_failing
Aug 26, 2024
Merged

fix: app hangs on starlette when sufficiently many cookies are set#751
maartenbreddels merged 2 commits intomasterfrom
08-26-ci_automatic_testing_suite_failing

Conversation

@iisakkirotko
Copy link
Copy Markdown
Collaborator

Couldn't reproduce locally. Giving ci a shot.

Copy link
Copy Markdown
Collaborator Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @iisakkirotko and the rest of your teammates on Graphite Graphite

@maartenbreddels
Copy link
Copy Markdown
Contributor

image

From https://github.com/widgetti/solara/actions/runs/10484645065/job/29169659662?pr=743

I added branch protection so we can auto merge, but that didn't let us do the auto commit. I had to disable it, so the updates ran on master. Maybe that was the same issue?

@EwoutH could you try to open a new trivial PR so we can test this?

In version 13.0 of `websockets`, the environmental variables to configure websockets were renamed (see [here](https://websockets.readthedocs.io/en/stable/project/changelog.html#id2)), which caused apps to hang when running on starlette with more than 8kb of cookies set.
@iisakkirotko iisakkirotko force-pushed the 08-26-ci_automatic_testing_suite_failing branch from 36a9641 to 165841b Compare August 26, 2024 11:35
@iisakkirotko iisakkirotko changed the title ci: automatic testing suite failing fix: app hangs on starlette when sufficiently many cookies are set Aug 26, 2024
@EwoutH
Copy link
Copy Markdown
Contributor

EwoutH commented Aug 26, 2024

@EwoutH could you try to open a new trivial PR so we can test this?

Of course, but I now see CI passing, so is it still needed?

Edit: If so, what do I need to do exactly?

@iisakkirotko iisakkirotko marked this pull request as ready for review August 26, 2024 12:51
@iisakkirotko
Copy link
Copy Markdown
Collaborator Author

Hey @EwoutH! You're right, this issue turned out to be unrelated. I'll ping you to test stuff when I get around to working on #750 :)

@maartenbreddels maartenbreddels merged commit 4647131 into master Aug 26, 2024
@maartenbreddels
Copy link
Copy Markdown
Contributor

Thanks @iisakkirotko !

Sorry, I misunderstoof @EwoutH , it seems 3 (!) related issues were confused in my mind

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