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

Cleanup files uploaded #266

Merged
merged 4 commits into from
Apr 12, 2022
Merged

Cleanup files uploaded #266

merged 4 commits into from
Apr 12, 2022

Conversation

LeSpocky
Copy link
Contributor

@LeSpocky LeSpocky commented Apr 7, 2022

Requirements for Contributing a Bug Fix

Identify the Bug

#264

Description of the Change

The list of files lives in the httpserver::http_request object. The destructor is called after the request handler is completed, so we can assume user had done all tasks needed for this request. Simply cleanup those files, before the files map is gone.

Alternate Designs

none considered

This is the most simple approach.

Possible Drawbacks

If the user needs those files, she should copy/move theme in the request
handler.

Verification Process

Integration tests in my application, not public.

Release Notes

"N/A" because follow-up to #257.

If the user needs those files, she should copy/move theme in the request
handler.

References: etr#264
Signed-off-by: Alexander Dahl <[email protected]>
@LeSpocky
Copy link
Contributor Author

LeSpocky commented Apr 7, 2022

Throwing this in as a draft, to see if this goes in the right direction.

  1. Not entirely sure about that friend class thing. I suppose the destructor should be private, because the constructors are also private?
  2. Need to come up with some way to test this in libhttpserver integration tests, right?

@LeSpocky
Copy link
Contributor Author

LeSpocky commented Apr 8, 2022

Modified the integration tests to check on deleted files. Hope this is sufficient to show the mechanism works as it should.

@LeSpocky
Copy link
Contributor Author

LeSpocky commented Apr 8, 2022

Modified the integration tests to check on deleted files. Hope this is sufficient to show the mechanism works as it should.

On some machines those tests fail (and sometimes with a strange 0!=0 result) and on some they don't.

Can this happen because the webserver runs in a different thread than the test and this is some kind of concurrency problem?

The webserver runs in a different thread than the test, therefor
deleting the uploaded files and checking if they are deleted might lead
into a race condition, and thus into tests failing sometimes, but not
always.

Moving the webserver stop and destructor behind the curl action should
be safe, every information needed for the test is copied to or in the
resource handler.
@LeSpocky LeSpocky marked this pull request as ready for review April 8, 2022 13:32
@etr
Copy link
Owner

etr commented Apr 12, 2022

Hey,

Thanks for the change.

Not entirely sure about that friend class thing. I suppose the destructor should be private, because the constructors are also private?

Yes, it is fine with the destructor being private and using friends to access it.

Need to come up with some way to test this in libhttpserver integration tests, right?

I guess you have the test now or is there something else you were thinking?

Can this happen because the webserver runs in a different thread than the test and this is some kind of concurrency problem?

It might be that the check is happening before the deletion has happened. Is this still happening or your latest change has fixed it?

@LeSpocky
Copy link
Contributor Author

Thanks for looking into my changes. Answers to your questions below.

Need to come up with some way to test this in libhttpserver integration tests, right?

I guess you have the test now or is there something else you were thinking?

Correct. I added the changes to the tests few days after the work in the destructor.

Can this happen because the webserver runs in a different thread than the test and this is some kind of concurrency problem?

It might be that the check is happening before the deletion has happened. Is this still happening or your latest change has fixed it?

No, the last change to the tests fixed that. CI did complain before, but not anymore after that.

@etr
Copy link
Owner

etr commented Apr 12, 2022

Thanks again for the help. All approved.

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.

2 participants