Skip to content

fix(mem): allocation checks and request abort on failed allocation #57

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 6, 2025
Merged

fix(mem): allocation checks and request abort on failed allocation #57

merged 2 commits into from
Feb 6, 2025

Conversation

mathieucarbou
Copy link
Member

No description provided.

@mathieucarbou mathieucarbou force-pushed the fix/allocation branch 4 times, most recently from 7a4e020 to 1ce49a5 Compare February 5, 2025 09:08
@mathieucarbou mathieucarbou marked this pull request as ready for review February 5, 2025 18:50
@mathieucarbou mathieucarbou changed the title fix(mem): abort request on failed allocations fix(mem): allocation checks and request abort on failed allocation Feb 5, 2025
@mathieucarbou mathieucarbou force-pushed the fix/allocation branch 3 times, most recently from 7360f9c to eff6cb7 Compare February 6, 2025 10:02
mathieucarbou
mathieucarbou commented Feb 6, 2025
View reviewed changes
mathieucarbou
mathieucarbou commented Feb 6, 2025
View reviewed changes
mathieucarbou
mathieucarbou commented Feb 6, 2025
View reviewed changes
mathieucarbou
mathieucarbou commented Feb 6, 2025
View reviewed changes
Copy link
Member Author

@mathieucarbou mathieucarbou left a comment

Choose a reason for hiding this comment

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

@me-no-dev @vortigont : I added a second commit to the PR to adjust the changes based on last comments.

I agree that we should not check for null after new Class. Either it is allocated, or program terminates or throw.

We could use new (std:::nothrow) Class to allow returning null.

Right now ESPAsyncWS does not use this mechanism anywhere, and within a request lifecycle, place where it would be important to abort() , we are already using a lot of new Class, for example to create responses.

So refactoring the project to support new (std:::nothrow) Class at many places is not possible and would also break the API because users are expecting a response object when the can beginRespsonse()

So I propose we settle on checking for null allocations only where really applicable (i.e. malloc or new arrays(

@mathieucarbou
fix(mem): memory management improvements:
fea2ec3 
- abort request on failed allocations
- std::vector => std::list for _pathParams
- limit the use of std::vector in middleware code that could execute in the context of a request
@mathieucarbou
Further adjustments
18cb706
@mathieucarbou mathieucarbou merged commit 7a42cb2 into main Feb 6, 2025
21 checks passed
@mathieucarbou mathieucarbou deleted the fix/allocation branch February 6, 2025 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@me-no-dev me-no-dev me-no-dev approved these changes

@vortigont vortigont Awaiting requested review from vortigont

Assignees

@mathieucarbou mathieucarbou

@me-no-dev me-no-dev

Labels
Status: Pending Merge Type: Improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mathieucarbou @me-no-dev @vortigont