Re: [RFC] Stream Error Handling Improvements

Hi Bob, Thanks for the feedback, the replies here also cover the feedback from Jordi and Ignace. > Why have you chosen a big list of constants rather than enums? > I think we should push for enums where applicable, meaning enum > StreamErrorMode, enum StreamErrorStore, enum StreamError. > Ok I will change it. StreamErrorMode and StreamErrorStore should be straightforward. The code one should be probably named StreamErrorCode (to not interact with what Jordi proposed) and think it will need to be backed as there should be a straightforward way how to compare it with exception code. I might need to extend the gen stub as I'm not sure it allows macro import for enum value like for constants or I will need to come up with some mapping. I just don't wan to duplicate the numbers. I will figure something out. > You define what a terminal error is, but does it correlate to specific > error codes? Like, is a given error code always terminal or not > terminal? Or can some error codes sometimes be terminal or not terminal > depending on context? > I've tried to look at the code, but I don't quite understand it, e.g. > php_plain_files_rename: When chown or chmod fail, the error is terminal. > When the copying fails (did you forget to adapt php_copy_file_ctx?), > that's non-terminal, though. Reading through the code, I have the > feeling that operations which do not call out to other operations, which > can error, are terminal. And all others are non-terminal. E.g. failing > to open a file is terminal. That the copying where the file opening is > part of, failed, is non-terminal. > And then *additionally* some operations, which don't prevent success are > also marked non-terminal - squeezing it into the boolean for the purpose > of not erroring. > Which makes some sense to me, but the description in the RFC is really > confusing here. And the meanings are muddied. > So currently the terminal / non terminal does not attach to code but it could be done. The current logic is really just supposed to reflect what is there already so the exception can be thrown in places where the function would fail. So the errors after which the function fails are terminal and the errors where the function still returns success are non terminal. Thus, if I understand that correctly, there should be an unique mapping > from error code to terminal. If we were to go with enums, we could make > it trivially a property of the individual enum values instead. > I think making it property of enum make more sense. Then I can drop that boolean property. > Should StreamException be attached the non-terminal errors which are > caused by the terminal errors? I.e. StreamException clearly is > STREAM_ERROR_CODE_PERMISSION_DENIED for example, but it happens during a > copy, so, should the information that a STREAM_ERROR_CODE_COPY_FAILED > was caused by that, also be present on the StreamException? > I think I get what you mean from the user point of view but I'm not sure how could this be reasonably linked. I could maybe check during stream error if there is EG(exception) it's a StreamException and if so, I could the update it with this error (e.g. some property that would hold those error can could be retrieved). I guess it could be potentially extended for terminal errors as well so it could have method like getAllErrors which would return all errors because sometimes there can be more terminal errors and some of them might get lost (last one will be added). However this would work just for exception so maybe more generic solution is needed here (see below). > Are stream errors which happen as part of a fully wrapped functionality > marked as wrapper error as well? Like if the server sent an invalid TLS > response to a file_get_contents("https://...") > operation, making the > whole operation fail? You obviously don't have access to the > intermediary internal stream resource there, where the stream error is > attached. > I actually thought about this yesterday that it's not ideal that some errors might get lost in this way so I think it should go to wrapper errors as well. > Is this what the logged errors section is about? Is that section talking > about "php_stream_display_wrapper_errors", which can concat some errors? > I see that this logic is mostly retained. > Yeah the logged errors is an existing mechanism for grouping errors to a single one (concatenating message). The display is a bit messy because it separated them by new line or <br/> (if html errors enabled) but I just kept it to limit BC impact. > Do I understand the implementation correctly, that user error handlers > can be called multiple times for a same operation? E.g. when copying a > file, you'll get first a PERMISSION_DENIED then a COPY_FAILED? Should > these not be somehow merged? Especially with global user_error handlers, > these cannot be aware of the fact that the permission denied is part of > anything else - at least not until possibly a second user_error > invocation happens. > This is somehow related to the exception error grouping as it requires some sort of linking between the errors. I guess to make this work we would need some top level wrapping similar to what logged errors do but instead of concatenating, it would group the errors in a better way which could then be used in exception or handler. This would, however, increase the complexity significantly as it is possible to nest the stream calls (e.g. stream calls in notification callback or in the new error handler) so there would probably need to be some stack. Also I'm not sure about the API. Should we pass always array of errors to the callback (which would mostly have just one element anyway or somehow chain the StreamError object (something like previousError()) and what message should be used in exception (e.g. last error and then helper function to get all errors)? I'm willing to look into this but would like to know if this is something important for user space and what is the usual flow how could this be used in frameworks and so on. In other words I would like to get more input to make sure that this is really needed as it requires quite a bit of effort to do. > Also, is the internal resource used for e.g. file_get_contents() leaked > into user error handlers? I'm not sure whether that's desirable. If > that's intended, that's fine too, I think. > > Currently yes. I need to actually double check the consequences of that as I didn't really think about internal resources. > Further suggestion on stream_get_errors() - why not return a proper > object instead of a loosely typed array? Proper typing, and creation is > actually faster. Also "docref" is so... antiquated? maybe just > "function" and have it contain the function name of the current operation. > > Yeah I will go for it and introduce StreamError as Jordi suggested with slight modification to use enum and dropping terminal and docref. > Does stream_get_errors() ever reset? Looking at the code it doesn't seem > so. It will just forever grow. E.g. fwrite() will report an error every > single time when the buffer is full and a write is attempted. If errors > aren't freed, that's just a memory leak (as long as the socket lives). > This is a serious flaw. > > Lastly, stream_get_errors() is overkill in a lot of cases (especially > when using fread()/fwrite() on simple sockets). I just care about the > last error code (there's just going to be one anyway). I'd love having a > stream_get_last_error() returning just the code. > > I guess this makes more sense but it kind of also implies the top level wrapping as it should contain all errors in the call. Or alternatively it could be just a ring with limited number of errors be behave in the same way like openssl_error_string. I guess it depends if we go with that top level wrapping. Cheers, Jakub