You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2020/06/09 16:36:28 UTC
[GitHub] [qpid-dispatch] kgiusti opened a new pull request #757: DISPATCH-1685: add heap allocation API that aborts on failure
kgiusti opened a new pull request #757:
URL: https://github.com/apache/qpid-dispatch/pull/757
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] asfgit closed pull request #757: DISPATCH-1685: add heap allocation API that aborts on failure
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #757:
URL: https://github.com/apache/qpid-dispatch/pull/757
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] kgiusti commented on a change in pull request #757: DISPATCH-1685: add heap allocation API that aborts on failure
Posted by GitBox <gi...@apache.org>.
kgiusti commented on a change in pull request #757:
URL: https://github.com/apache/qpid-dispatch/pull/757#discussion_r437621702
##########
File path: include/qpid/dispatch/ctools.h
##########
@@ -217,4 +218,41 @@ do { \
#define MIN(a,b) (((a)<(b))?(a):(b))
#define MAX(a,b) (((a)>(b))?(a):(b))
+//
+// Heap allocation with abort() on failure
+//
+
+static inline void *qd_malloc(size_t size)
Review comment:
Ah - good point. I'm pretty sure there's no zero size use cases, but I'll add an assert() to enforce that contract.
If size == 0 the caller shouldn't make the call at all, or fall back to using malloc directly if they're doing something... weird.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] kgiusti commented on pull request #757: DISPATCH-1685: add heap allocation API that aborts on failure
Posted by GitBox <gi...@apache.org>.
kgiusti commented on pull request #757:
URL: https://github.com/apache/qpid-dispatch/pull/757#issuecomment-641409980
> Should the existing code be modified to use these new functions or is that another commit ?
I'm not planning any further commits at present. I just wanted to propose the new feature and get feedback on the idea of abort() on malloc failure in the router.
If the consensus is that this is A Good Idea then we can either phase these in over time (easy) or big bang (huge amount of work). I'll leave that on the table for further discussion.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #757: DISPATCH-1685: add heap allocation API that aborts on failure
Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #757:
URL: https://github.com/apache/qpid-dispatch/pull/757#discussion_r437586665
##########
File path: include/qpid/dispatch/ctools.h
##########
@@ -217,4 +218,41 @@ do { \
#define MIN(a,b) (((a)<(b))?(a):(b))
#define MAX(a,b) (((a)>(b))?(a):(b))
+//
+// Heap allocation with abort() on failure
+//
+
+static inline void *qd_malloc(size_t size)
+{
+ void *ptr = malloc(size);
+ if (!ptr)
+ abort();
Review comment:
maybe some error message for the aborts? on the other hand, that abort will probably never be called in practice, so it does not matter for UX...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #757: DISPATCH-1685: add heap allocation API that aborts on failure
Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #757:
URL: https://github.com/apache/qpid-dispatch/pull/757#discussion_r437586665
##########
File path: include/qpid/dispatch/ctools.h
##########
@@ -217,4 +218,41 @@ do { \
#define MIN(a,b) (((a)<(b))?(a):(b))
#define MAX(a,b) (((a)>(b))?(a):(b))
+//
+// Heap allocation with abort() on failure
+//
+
+static inline void *qd_malloc(size_t size)
+{
+ void *ptr = malloc(size);
+ if (!ptr)
+ abort();
Review comment:
maybe some error message for the abort? on the other hand, that abort will probably never be called in practice, so it does not matter for UX...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] kgiusti commented on a change in pull request #757: DISPATCH-1685: add heap allocation API that aborts on failure
Posted by GitBox <gi...@apache.org>.
kgiusti commented on a change in pull request #757:
URL: https://github.com/apache/qpid-dispatch/pull/757#discussion_r437615900
##########
File path: include/qpid/dispatch/ctools.h
##########
@@ -217,4 +218,41 @@ do { \
#define MIN(a,b) (((a)<(b))?(a):(b))
#define MAX(a,b) (((a)>(b))?(a):(b))
+//
+// Heap allocation with abort() on failure
+//
+
+static inline void *qd_malloc(size_t size)
+{
+ void *ptr = malloc(size);
+ if (!ptr)
+ abort();
Review comment:
I was thinking about that, but decided that if a hard memory limit is hit it is unlikely that doing any I/O would succeed.
But at this point it probably cannot hurt to call perror before aborting... at best it will indicate the failure was due to low memory. I'll update the patch.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] ganeshmurthy commented on pull request #757: DISPATCH-1685: add heap allocation API that aborts on failure
Posted by GitBox <gi...@apache.org>.
ganeshmurthy commented on pull request #757:
URL: https://github.com/apache/qpid-dispatch/pull/757#issuecomment-641348023
Should the existing code be modified to use these new functions or is that another commit ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #757: DISPATCH-1685: add heap allocation API that aborts on failure
Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #757:
URL: https://github.com/apache/qpid-dispatch/pull/757#discussion_r437588135
##########
File path: include/qpid/dispatch/ctools.h
##########
@@ -217,4 +218,41 @@ do { \
#define MIN(a,b) (((a)<(b))?(a):(b))
#define MAX(a,b) (((a)>(b))?(a):(b))
+//
+// Heap allocation with abort() on failure
+//
+
+static inline void *qd_malloc(size_t size)
Review comment:
can it ever happen in dispatch that size==0 in calling this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] kgiusti commented on pull request #757: DISPATCH-1685: add heap allocation API that aborts on failure
Posted by GitBox <gi...@apache.org>.
kgiusti commented on pull request #757:
URL: https://github.com/apache/qpid-dispatch/pull/757#issuecomment-641482542
In general, malloc et. al. will fail (return null) in the case where the process hits RLIMIT_AS/DATA. I believe it can also happen when running in a container that has a total memory limit configured.
Otherwise linux will return a non-zero value even if there is _no memory currently available_. When the pointer is de-referenced the process hits a page fault and is suspended until more memory becomes available (or it is OOM killed).
So if abort() occurs it is a hint to a Carbon-Based Lifeform that the provisioned memory is insufficient for the use case.
Or there's a bug.... there's always bugs.....
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org