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

Fix HttpUpgrader duplicate stream de-allocation #2722

Merged
merged 5 commits into from
Mar 7, 2024

Conversation

mikee47
Copy link
Contributor

@mikee47 mikee47 commented Mar 6, 2024

This PR fixes the issue raised in #2720, where a stream object gets free'd twice resulting in unpredictable behaviour.

Looks like this bug was introduced in #2643.

@slaff slaff added this to the 5.2.0 milestone Mar 6, 2024
@SmingHub SmingHub deleted a comment from what-the-diff bot Mar 6, 2024
@mikee47 mikee47 changed the title HttpUpgrader must relinquish ownership of stream to request Fix HttpUpgrader duplicate stream de-allocation Mar 6, 2024
@mikee47
Copy link
Contributor Author

mikee47 commented Mar 6, 2024

Looks like RbootHttpUpdater is obsolete and duplicate code. Should we remove it?

@slaff
Copy link
Contributor

slaff commented Mar 6, 2024

Looks like RbootHttpUpdater is obsolete and duplicate code. Should we remove it?

Yes, but I would prefer to do it in a major version. Maybe I should open a ticket for the 6.0.0 version and add this one ...

@mikee47
Copy link
Contributor Author

mikee47 commented Mar 6, 2024

One other thing is that the itemComplete and updateComplete methods of HttpUpgrader are virtual, but I see no overrides for these elsewhere. Safest option would be to remove virtual...?

@slaff slaff merged commit d9d97bd into SmingHub:develop Mar 7, 2024
46 checks passed
@mikee47 mikee47 deleted the fix/HttpUpgrader-stream branch March 7, 2024 09:49
@slaff slaff mentioned this pull request May 8, 2024
5 tasks
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.

2 participants