Skip to content

Conversation

@coin0
Copy link

@coin0 coin0 commented Jan 29, 2019

This is a PR for additional private data support for deferred_response, it is necessary to carry private information so that cycle_callback could tell different HTTP connections by the customized data. cleanup_callback is used to clean up private data allocated by users.

@codecov-io
Copy link

Codecov Report

Merging #144 into master will decrease coverage by 0.03%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #144      +/-   ##
==========================================
- Coverage   96.46%   96.42%   -0.04%     
==========================================
  Files          34       34              
  Lines        2995     2997       +2     
==========================================
+ Hits         2889     2890       +1     
- Misses        106      107       +1
Impacted Files Coverage Δ
test/integ/deferred.cpp 100% <100%> (ø) ⬆️
src/deferred_response.cpp 83.33% <100%> (ø) ⬆️
src/httpserver/deferred_response.hpp 80% <50%> (-20%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ead9ccc...8987567. Read the comment docs.

};

typedef ssize_t(*cycle_callback_ptr)(char*, size_t);
typedef ssize_t(*cycle_callback_ptr)(void*, char*, size_t);
Copy link
Owner

Choose a reason for hiding this comment

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

When I was thinking about this feature, I tried to figure how we could avoid the "void*" for the type.

To be honest this is probably one of those cases where "seeing it after it is written might help".

I was thinking we could use a template type to define the first parameter and a "shared_ptr" to pass it through. This would save us from having to use the "void*" and, ideally, from having the cleanup function entirely.

To be honest, I would convert the currently existing "char*" into a shared_ptr if we are perturbing the interface.

const shared_ptr<http_response> render_GET(const http_request& req)
{
return shared_ptr<deferred_response>(new deferred_response(test_callback, "cycle callback response"));
return shared_ptr<deferred_response>(new deferred_response(test_callback, nullptr, nullptr,
Copy link
Owner

Choose a reason for hiding this comment

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

We should have a separate resource and a separate that is used to verify that the new functionality is working fine.


ssize_t test_callback (char* buf, size_t max) {
if (counter == 2) {
typedef struct {
Copy link
Owner

Choose a reason for hiding this comment

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

No need for typedefs - we can just have a named struct

const std::shared_ptr<http_response> render_GET(const http_request& req) {
return std::shared_ptr<deferred_response>(new deferred_response(test_callback, "cycle callback response"));
// private data of new connections
auto priv_data = new connection();
Copy link
Owner

Choose a reason for hiding this comment

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

Please, refrain from using "auto" for now

@etr
Copy link
Owner

etr commented Jan 30, 2019

Hey,

thanks for your first contribution.

I left you some comments on the code - hopefully, we can work out a good solution for this (it is something I was looking at given a previous pull request).

Besides the comments, can you please follow one of the templates for the pull request?

@etr etr added feature-request Feature requests or enhancements duplicate Issues which are duplicates of other issues, i.e. they have been reported before. needs-review Pull requests which need code review, and approval from maintainers. under-review Pull requests being reviewed by maintainers requires-changes Pull requests which need to be updated based on review comments and then reviewed again. labels Jan 30, 2019
@etr
Copy link
Owner

etr commented Feb 11, 2019

I have an implementation of this that I completed over the weekend (see: #145 ) - let me know if there is still something I am missing from your use-case.

@etr etr closed this Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

duplicate Issues which are duplicates of other issues, i.e. they have been reported before. feature-request Feature requests or enhancements needs-review Pull requests which need code review, and approval from maintainers. requires-changes Pull requests which need to be updated based on review comments and then reviewed again. under-review Pull requests being reviewed by maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants