search

ohler55 / agoo

A High Performance HTTP Server for Ruby
MIT License
909 stars 39 forks source link

Completion of error handling #33

Closed elfring closed 5 years ago

elfring commented 5 years ago

Would you like to add more error handling for return values from functions like the following?

ohler55 commented 5 years ago

Cool, a code review!

elfring commented 5 years ago

I suggest to avoid ignorance of return values a bit more. Would you like to detect every error situation as early as possible?

ohler55 commented 5 years ago

I believe this has been addressed. Can it be closed?

elfring commented 5 years ago

How will the integration of better source code analysis tools evolve for your software development work flow so that remaining update candidates will be pointed out?

ohler55 commented 5 years ago

I have tried CppCheck with some success but it gives false error warnings enough times that I don't want to or can't really make it an automated part of the build process. It is a useful tool for catching some errors though so I run it sometimes. What tool are you using that detected problems in the functions you mentioned?

As for the functions you pointed out, I made some changes to address what I think you might have been referring to.

agoo_log_init: Added a check for the memory allocation.

agoo_malloc: Added a check for NULL return from malloc. This function is only used in development for checking for memory leaks and writing out of bounds.

agoo_queue_multi_init: Added memory check.

graphql_load_file: added a check for the return value of fseek and ftell. All other errors are being checked and an exception raised on error.

The changes will go into the next release but are currently in the develop branch.

elfring commented 5 years ago

What tool are you using that detected problems in the functions you mentioned?

My own eyes together with GitHub's source code search interface.

As for the functions you pointed out, I made some changes to address what I think you might have been referring to.

Thanks for another software adjustment. The error detection and corresponding exception handling is still incomplete, isn't it?

ohler55 commented 5 years ago

I made some additional changes in debug.c.

ohler55 commented 5 years ago

More changes made based on your comments.

elfring commented 5 years ago

I hope that a few of my code review comments will get further constructive responses.

ohler55 commented 5 years ago

I believe I have addressed the code review comments with changes in the code.

elfring commented 5 years ago

Partly, yes. - Remaining update candidates will need further software development efforts if you would like to care for these open issues.

ohler55 commented 5 years ago

I'm not sure what other update candidates you are referring to.

elfring commented 5 years ago

Function calls for further considerations:

ohler55 commented 5 years ago

What should be considered on each of those?

ALLOC_N succeeds or raises an exception. Exceptions are currently passed back to the caller. localtime is supposed to always succeed according to the man pages time is use once in log.c for a timestamp. There is no other way to get the time so if it fails the timestamp is 1 second before 1970. There is no other way to report the error other than the log itself so any reporting would suffer the same error. Leaving it at one failure that is obvious in the logs is fine. pthread_mutex_lock we have had this discussion twice now. I've explained my reasons for not adding a check for every lock and unlock. Please accept that I do not intend to change the use of pthread_mutex.

elfring commented 5 years ago
ohler55 commented 5 years ago

ALLOC_N is a Ruby macro defined in ruby.h.

ohler55 commented 5 years ago

Can this be closed?

elfring commented 5 years ago

Not until the discussed exception handling will be complete.

ohler55 commented 5 years ago

Anything else?

elfring commented 5 years ago

Obviously, yes.

Further update candidates:

Will your software development attention grow for cross-cutting concerns?

ohler55 commented 5 years ago

Pushed fixes.

elfring commented 5 years ago

Can you get into the mood to tackle such cross-cutting concerns by any higher level software development means?

ohler55 commented 5 years ago

What exactly are you referring to? Can you be more specific in what you are proposing?

elfring commented 5 years ago

I became just curious again if you would like to reconsider previously mentioned information sources once more. Are you looking for encapsulation of exception handling so that the corresponding software maintenance can become eventually more convenient?

ohler55 commented 5 years ago

I have already used CppCheck and run it occasionally. I am not willing to make it part of the CI testing as it gives false positives sometimes. It is useful for periodic checks though. As for Coccinelle, I am comfortable with emacs and using emacs macros to make the kind of changes Coccinelle would help with so I don't see the need to use Coccinelle.

elfring commented 5 years ago
ohler55 commented 5 years ago

• I'll consider it but so far have not found the time. • I don't need a patch language for Agoo. It is not large enough at this point. As for the effectiveness of using emacs, I think that is a personal preference. It has worked well for me for a very long time.

elfring commented 5 years ago
ohler55 commented 5 years ago

• Our dialogs have resulted in nice adjustments. • How our roles evolve it yet to be seen. I do expect that you and others will continue to point out weaknesses.

elfring commented 5 years ago

I would appreciate when development tools can help more in reminding for such weaknesses and fixing them. But I imagine that social factors can hinder progress also in this direction besides general challenges around change willingness and support.

ohler55 commented 5 years ago

As with any project there will be differences of opinions as well. That should not be taken as an unwillingness to make changes.

elfring commented 5 years ago

I thank you again that you were willing to adjust this software to some degree (also without direct contribution of financial resources from me so far). You agreed to reduce return value ignorance in ways that are shown by the published commits. I imagine that we will reach a software development status where we will stumble on places for which you expressed change reluctance already around specific function calls.

ohler55 commented 5 years ago

So can this be closed now?

elfring commented 5 years ago

Not so far because there are still source code places which can look suspicious.

ohler55 commented 5 years ago

Why is that suspicious?

Do you intend for this issue to remain open forever? Usually issue are for a specific issue, not a general category like "make it better".

elfring commented 5 years ago

Why is that suspicious?

    for (i = agoo_server.thread_cnt, vp = the_rserver.eval_threads; 0 < i; i--, vp++) {
        *vp = rb_thread_create(wrap_process_loop, NULL);
    }
    *vp = Qnil;

Can you see a need for another check for the return value from such a function call?

Do you intend for this issue to remain open forever?

Not really. - But possible adjustments will depend on the clarification of the running source code analysis and a consensus on review results as usual.

Usually issue are for a specific issue, not a general category like "make it better".

I am unsure on how long it will take to achieve completeness for exception handling here.

ohler55 commented 5 years ago

In Ruby the general approach to handling errors is to raise an exception. This is once of those cases. If it fails an exception is raised. There is no need to check at this level. The calling application or the user of the library handles the exception as it sees fit.

Instead of leaving the issue open ended with a vague statement that says handle all exception cases how about identifying the places that you think are problems and when those are fixed the issue is closed. Otherwise with no criteria for completion the issue will have to be rejected as too vague.

elfring commented 5 years ago

If it fails an exception is raised.

Instead of leaving the issue open ended with a vague statement

Which details do you find too vague here?

that says handle all exception cases

Is this a desirable software development goal?

how about identifying the places that you think are problems

ohler55 commented 5 years ago

• That is correct. A ruby call that raises an exception does not return. Raising an exception does a long jump. • I don't know what you read about the Ruby C API so I don't know what you missed. It is easy to try though. Write a small app and raise and exception.

• It is a desirable goal to handle all exceptions just like it is desirable to not have any bugs but an issue that says "fix all bugs" isn't very helpful if the bugs are not identified.

• You identified CppCheck as a tool to use and I have used that. It can not be made part of CI as it also gives false positives. I think I have answered this several times now.

• The pthread mutex calls have been been discussed and I have give my reasons for not bothering to check the return codes. The fact that you do not agree with my assessment does not mean the issue should stay open forever. If those are the only identified remaining issue then this can be closed because they have been responded to.

• This comment is a perfect example of a vague issue. Formal verification ranges from minimal using tests on up to a complete mathematical proof the software is correct. Agoo has unit tests and that is the current verification process. A more concise and reasonable issue would be to describe a situation that caused a failure and ask for it to be fixed.

elfring commented 5 years ago

I don't know what you read about the Ruby C API so I don't know what you missed.

Where is the function “rb_thread_create” completely documented?

but an issue that says "fix all bugs" isn't very helpful if the bugs are not identified.

I am proposing the application of a general source code search pattern for a while which can eventually result also in aspect-oriented software development.

I think I have answered this several times now.

Would you like to fix the observation of false positives for discussed analysis tools anyhow?

The pthread mutex calls have been been discussed and I have give my reasons for not bothering to check the return codes.

ohler55 commented 5 years ago

Sadly, the Ruby C API is not well documented at all or at least I have not found a complete set of API descriptions. The source code is available though.

If you can explain how a "general source code search pattern" would be used and how it would fix some perceived issue then please explain in enough detail so that it can be used.

The false positives for CppCheck are an issue for the CppCheck maintainers but I have not submitted an issue to them yet. Not having submitted an issue is not an Agoo issue though.

I accepted your suggestions on issues where I agreed with you. That does not mean I will agree with you on all things you think are an issue. You can interpret that disagreement as a mistake as you wish but it would be nice if you would respect my opinion as well when we disagree.

ohler55 commented 5 years ago

So here is the deal. Provide a clear well defined issue that can be addressed or this issue will be closed. There are some helpful guidelines on issue submission that describe identify some good practices. Please take a look at https://github.com/magento/magento2/wiki/Issue-reporting-guidelines and/or https://github.com/necolas/issue-guidelines. Issues submissions that follow those guidelines I can address either by fixing or rejecting as not relevant.

elfring commented 5 years ago

Sadly, the Ruby C API is not well documented at all

Would we like to influence this software situation anyhow?

If you can explain how a "general source code search pattern" would be used

I am used to specific software weakness searches for years as you can determine such experiences from my public bug reports. Do you need any additional training in the mentioned directions?

and how it would fix some perceived issue then please explain in enough detail so that it can be used.

Other information sources explained this already in sufficient ways. It seems that you stumble on general challenges to integrate known advices from there.

… but I have not submitted an issue to them yet.

Would you dare to point further development considerations out to the maintainers of affected analysis and code transformation tools?

That does not mean I will agree with you on all things you think are an issue.

This view is fine in principle.

… but it would be nice if you would respect my opinion

Provide a clear well defined issue that can be addressed

ohler55 commented 5 years ago

Ruby is open source so anyone can contribute to it. If by influence you mean I should document the APIs then I am not ready to spend my time on that effort. If you mean thank someone else for doing it then that I can do.

You have pointed out a number of places where the code can be improved. I thank you for that. As for training in finding areas that can be improved I am always interested in improving my development skills but I am not interested in searching through other peoples code for weaknesses. I prefer to develop code instead.

I would certainly prefer to have each issue clearly defined. For example, if you were to create an issue indicating return values of pthread_mutex functions are not checked and could lead to concurrency issues that would be a reasonable issue. I would reject it with a similar description that I have given before but that is the preferred granularity and clarity for an issue. Give me something I can work with.

Another example might be around malloc use. I have already gone over the code and think all the return values from malloc are checked but I may have missed one somewhere. You could say, the malloc return value in file xxx.c line 123 is not checked or list the files and lines if there are multiple occurrences. You created a separate issue for the signal handler. That was perfect. A clear problem and discernible goal and success criteria.

elfring commented 5 years ago

If by influence you mean I should document the APIs …

I am just curious who might become motivated enough to adjust the source code of the documentation another bit for the involved software.

… skills but I am not interested in searching through other peoples code for weaknesses.

This information seems to indicate another significant difference between us.

I prefer to develop code instead.

We tend to distribute our software development capacity also in different ways.

I would certainly prefer to have each issue clearly defined.

It seems that I need to interpret such a wording as another indication from your personal variation of change resistance (which might have grown for your software components the more I pointed open issues out).

Give me something I can work with.

Repetition: pthread_mutex_init ⇒ agoo_server_setup

ohler55 commented 5 years ago

I am happy to make changes to fix issues. I think I have demonstrated that as I made a lot of changes related to this issue. You will note that each set of changes was prompted by a clearer description of that part of this issue. I think you will find most projects expect a description of what should be fixed that does not change as and expand as each change is made. Anyway lets close this and you can open up more explicit issues if you find something that you think should be addressed.

elfring commented 5 years ago

I am happy to make changes to fix issues.

Can such a view trigger activities for inspiration and help with additional evolution in related areas?

You will note that each set of changes was prompted by a clearer description of that part of this issue.

We came along different expectations for clearness.

Anyway lets close this

You should know also from our development discussion where this software implementation is still unfinished (and therefore improvable) so far, shouldn't you?

I guess that you are used to some tools for working with (regular) search patterns. When will similar search patterns be specified as pointcuts and corresponding advice (according to aspect-oriented software development)?

and you can open up more explicit issues if you find something that you think should be addressed.

Under which circumstances would you consider the application of scoped locks here?

ohler55 commented 5 years ago

Software is never finished. There is always something that can be done to improve or add new features.

I am familiar with some of the search too such as grep and have written my own helper utilities as well.

I have not seen the need for scoped locks. They are a relatively new feature of C++ so not really something I plan on using in a C only application even if there was a need for them.

elfring commented 5 years ago

They are a relatively new feature of C++ so not really something I plan on using in a C only application even if there was a need for them.

ohler55 commented 5 years ago

They are not needed. There is no fix needed that I am aware of.

No, I do not want to use any C++ lock features.

There is no choice in Ruby. The GVL must be used for the Ruby portion.