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

Closing procedure improvements, several fixes #334

Merged
merged 95 commits into from
Jun 20, 2017
Merged

Conversation

weinrank
Copy link
Member

This is a huge PR, sorry ... :)

Changes:

closing mechanism refactoring
If a peer closes the transport connection, this is now indicated by the on_close event and not by receiving no data when calling neat_read.
According to #316

callback handling
The callback handling has been improved in many aspects.
For example on_all_written is called every time a neat_write call has successfully delivered data to the transport layer and the flow buffer is drained.
See #272

improved memory management
The library should now handle memory a lot better, valgrind is much happier now. :)

==15298== 
==15298== HEAP SUMMARY:
==15298==     in use at exit: 0 bytes in 0 blocks
==15298==   total heap usage: 61,819 allocs, 61,819 frees, 6,215,102 bytes allocated
==15298== 
==15298== All heap blocks were freed -- no leaks are possible
==15298== 
==15298== For counts of detected and suppressed errors, rerun with: -v
==15298== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

misc changes

  • RUN_ONCE and many flows fails #291 should be fixed
  • examples are not installed systemwide by default
  • improved OpenSSL detection
  • examples cleaned (e.g. http client supports https, https client removed)
  • message preserving/stream property
  • code cleaning

@bagder
Copy link
Member

bagder commented Jun 19, 2017

To make this PR more reviewable and ready to merge, I would ask you to:

  1. squash commits so that you leave only the logical changes you want to show, there's no need for us to see your development history
  2. provide clear and descriptive commit messages that explain what you're changing and possibly why

Ideally, you can detect subsets of this PR that can and should be made into its own separate PRs instead, to make it easier to review and makes it easier for future debugging and bisecting.

@weinrank
Copy link
Member Author

I'm not familiar with squashing commits, many of my commits can't be linked to specific topic.
Would it hurt you much if I just merge the PR this time and take care in the future? :)

@weinrank
Copy link
Member Author

Would it help if I use "squash and merge"?

# Conflicts:
#	examples/client_http_get.c
@bagder
Copy link
Member

bagder commented Jun 20, 2017

Squashing == merging several related commits into one. Easiest done with git rebase -i and when you've narrowed it down to the set you like (and written clear commit messages), you force-push to this branch and we can review what you have.

I don't think it is acceptable for any project to push 93 commits with lacking commit messages and (virtually) no review, but I don't make the rules and I will accept whatever everyone thinks is suitable. (That's absolutely nothing personal against you nor your work, it's a question of what process we should use when merging code.)

"squash and merge" is probably a bit too simple for you since it will merge all commits into one, which feels it might be a bit too exaggerated in the other direction.

@weinrank
Copy link
Member Author

I totally agree with you that merging these changes without a review is not a good way but we discussed this and agreed on just merge the changes.

@weinrank weinrank merged commit 55163c0 into master Jun 20, 2017
This was referenced Jun 20, 2017
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