On Tue, 29 Aug 2023 12:56:35 GMT, Johan Vos <j...@openjdk.org> wrote:
>> The idea is to avoid the crash entirely. If we actually hit this case, it is >> very likely that other calls will also run out of memory. Returning to Java >> as quickly as possible will let any pending OOME be thrown. A library should >> not exit, so really we have two choices here: >> >> 1. Throw OOM and then return >> 2. Just return >> >> While option 1 might be the better choice, it would be a more intrusive fix. >> Most of the native code just returns to Java, although we do have a few >> places where we throw. OOME. It might be better to keep this fix simple (and >> more in line with what other functions in glass do), and address this with a >> follow-up issue? > > I'm not against that, especially since it's in line with what we do in other > functions in glass. > However, I am worried about the consequences. In case we just return, the > caller has no idea that there is a major problem. A Runnable is supplied to > e.g. _invokeAndWait, but it will never get executed while the caller (and the > application logic) assumes it is scheduled. This can have serious > consequences and unexpected behavior in the application. > But maybe I'm missing something and it is less severe than I'm picturing it? I can see what you are saying, but worth noting in this specific case is that if the malloc of `RunnableContext` (a 12-byte struct) fails, we're not going to be able to allocate an OOME anyway. My preference would be to leave this fix as is, and file a follow-up issue to change the return type of `GtkApplication::submitForLaterInvocation` (and the equivalent methods in the other glass pipelines) to `boolean` so we can return an error code and throw an exception (which would very likely provoke an OOME, but in any case would never silently fail). ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1223#discussion_r1308861489
