Skip to content

Conversation

itamarst
Copy link

@itamarst itamarst commented May 22, 2025

❓ What kind of change does this PR introduce?

  • 🐞 bug fix
  • 🐣 feature
  • πŸ“‹ docs update
  • πŸ“‹ tests/coverage improvement
  • πŸ“‹ refactoring
  • πŸ’₯ other

❓ What is the current behavior? (You can also link to an open issue here)

When connections can't be handled, the connection is just closed.

❓ What is the new behavior (if this is a feature change)?

A 503 error is returned, as suggested in TODO comment in relevant part of the code.

Since writing to the socket is blocking, this is done in a thread.

πŸ“‹ Other information:

For some reason OpenStack Ironic needs this apparently, I didn't dig into why.

πŸ“‹ Contribution checklist:

(If you're a first-timer, check out
this guide on making great pull requests)

  • I wrote descriptive pull request text above
  • I think the code is well written
  • I wrote good commit messages
  • I have squashed related commits together after
    the changes have been approved
  • Unit tests for the changes exist
  • Integration tests for the changes exist (if applicable)
  • I used the same coding conventions as the rest of the project
  • The new code doesn't generate linter offenses
  • Documentation reflects the changes
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences

This change is Reviewable

This comment was marked as outdated.

@itamarst itamarst marked this pull request as draft May 23, 2025 13:29
@itamarst itamarst force-pushed the 503-when-overloaded branch 2 times, most recently from 05f2bee to 57206a1 Compare May 23, 2025 13:47
@itamarst itamarst force-pushed the 503-when-overloaded branch 3 times, most recently from ead7721 to 97a6317 Compare May 28, 2025 17:24
@itamarst
Copy link
Author

So now it ... fails, but not as consistently? I don't really know.

@itamarst
Copy link
Author

Any other thoughts? It's pretty annoying that it passes some of the time but not all the time.

@itamarst itamarst force-pushed the 503-when-overloaded branch from 45698b9 to 4a5abc5 Compare June 2, 2025 15:54
@itamarst itamarst marked this pull request as ready for review June 2, 2025 16:05
@itamarst
Copy link
Author

itamarst commented Jun 2, 2025

Looks like we're down to only 3.14 failing, which were already failing on main so hopefully unrelated to my changes.

@itamarst itamarst force-pushed the 503-when-overloaded branch from c532189 to b460ce4 Compare June 17, 2025 16:41
@itamarst
Copy link
Author

itamarst commented Jun 17, 2025

Failures are due to 3.14 and #747, unrelated to this PR as far as I can tell.

@itamarst itamarst requested a review from webknjaz June 17, 2025 17:03
@webknjaz webknjaz force-pushed the 503-when-overloaded branch 3 times, most recently from e4edaae to dab349d Compare June 22, 2025 21:56
@webknjaz
Copy link
Member

@itamarst i've hit rebase on GH UI post a few CI fixes to main and it looks like the new test is flaky in there: https://github.com/cherrypy/cheroot/actions/runs/15811177788/job/44562688410?pr=745#step:15:1574

@itamarst
Copy link
Author

Maybe that's why the linger was in the code I copied from. I'll test that out next week.

@itamarst itamarst force-pushed the 503-when-overloaded branch from dab349d to 8edc162 Compare June 23, 2025 15:10
@itamarst
Copy link
Author

Seems better maybe with linger added back?

@itamarst itamarst force-pushed the 503-when-overloaded branch from 8edc162 to 71f7254 Compare July 9, 2025 16:31
@itamarst
Copy link
Author

itamarst commented Jul 9, 2025

@webknjaz back to you!

@webknjaz
Copy link
Member

@itamarst one thing I forgot to ask for is adding a change note for Towncrier. Could you do that?
https://cheroot.cherrypy.dev/en/latest/contributing/guidelines/#adding-change-notes-with-your-prs

@webknjaz
Copy link
Member

@itamarst I think this is mostly ready, short of the requests above. When writing a change note, https://cheroot--745.org.readthedocs.build/en/745/history/#to-be-included-in-the-next-release can be used to preview rendering and compare the writing style with the surrounding text.

@webknjaz webknjaz added the enhancement Improvement label Jul 11, 2025
@itamarst itamarst force-pushed the 503-when-overloaded branch from 71f7254 to fd19874 Compare July 11, 2025 13:25
@itamarst
Copy link
Author

OK back to you @webknjaz

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Wonderful! Thanks for the contrib.

@webknjaz webknjaz merged commit 587d1fa into cherrypy:main Jul 11, 2025
69 of 76 checks passed
@webknjaz webknjaz removed the triage label Jul 11, 2025
@webknjaz
Copy link
Member

webknjaz commented Oct 3, 2025

Looks like we have a regression coming out of this change: #769 / #778

# We can't just raise an exception because that will kill this
# thread, and prevent 503 errors from being sent to future
# connections.
self.server.error_log(

Choose a reason for hiding this comment

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

There is no self.server within HTTPServer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants