KAFKA-14012: Add warning to closeQuietly documentation about method references of null objects

[vamossagar12]

Utils.closeQuietly method accepts AutoCloseable object, and close it and sometimes, method references of null objects are passed to it which causes npes. This PR fixes it..

[vamossagar12]

@C0urante , @showuon This is a very simple fix. I didn't add any tests for it as I didn't think it's necessary. Let me know if you think need a test for this?

[C0urante]

I think there's a better way to go about this. The general issue we're trying to address is that accessing null references can throw NPEs; there's nothing very special about the case of method references and Utils::closeQuietly.

If we were to follow the logic in this PR as-is, we'd have to add if (thing != null) checks around every method invocation on thing, which would have to happen here, here, here, here, etc.

A less-verbose approach that'd be just as safe is to rely on guarantees we can get at construction time for these objects:

• The configLog field of KafkaConfigBackingStore is declared final and initialized in the constructor; we can easily verify that it will never be null
• The offsetStore field of AbstractWorkerSourceTask is also declared final, and accepted as a constructor parameter. Since we never pass in null as the offsetStore parameter to the constructor for this class, we can add a call to Objects::requireNonNull in the constructor that won't break any existing code, and will obviate the need for null checks in other parts of the class

The reason we have a special case for this in WorkerConnector is that its offsetStore can be null, since that class is used for both source connectors (which an offset store is brought up for) and sink connectors (which no offset store is brought up for).

[vamossagar12]

I think there's a better way to go about this. The general issue we're trying to address is that accessing null references can throw NPEs; there's nothing very special about the case of method references and Utils::closeQuietly.

If we were to follow the logic in this PR as-is, we'd have to add if (thing != null) checks around every method invocation on thing, which would have to happen here, here, here, here, etc.

A less-verbose approach that'd be just as safe is to rely on guarantees we can get at construction time for these objects:

• The configLog field of KafkaConfigBackingStore is declared final and initialized in the constructor; we can easily verify that it will never be null
• The offsetStore field of AbstractWorkerSourceTask is also declared final, and accepted as a constructor parameter. Since we never pass in null as the offsetStore parameter to the constructor for this class, we can add a call to Objects::requireNonNull in the constructor that won't break any existing code, and will obviate the need for null checks in other parts of the class

The reason we have a special case for this in WorkerConnector is that its offsetStore can be null, since that class is used for both source connectors (which an offset store is brought up for) and sink connectors (which no offset store is brought up for).

Thanks for the detailed explanation Chris! I think this suggestion of yours is a lot neater. Also, I realised that on similar lines, I don't need a null check for task object in AbstractWorkerSourceTask as upstream it seems to be ensured that it would be setup properly (or throw an exception if the taskClass isn't proper). Let me know if this makes sense.

[C0urante]

LGTM! (With some non-blocker suggestions)

I wonder if there are some higher-level fixes we might also adopt that would make NPEs from method references harder to write. Some ideas that come to mind:

1. Stop using method references where possible and start actually implementing the AutoCloseable interface (this seems reasonable for the *BackingStore interfaces given that they already have stop() methods that behave closely to AutoCloseable::close, for example).
2. Add a new variant to Utils::closeQuietly that accepts a possibly-null object and some kind of consumer that can be applied to that object if it's non-null that will close it; I'm imagining calling it would look something like closeQuietly(producer, p -> p.close(duration), "source task producer"); instead of the existing logic for closing a source task's producer, or closeQuietly(offsetStore, OffsetBackingStore::stop, "offset backing store") instead of the existing logic for stopping a source task's offset store.

[vamossagar12]

LGTM! (With some non-blocker suggestions)

I wonder if there are some higher-level fixes we might also adopt that would make NPEs from method references harder to write. Some ideas that come to mind:

1. Stop using method references where possible and start actually implementing the AutoCloseable interface (this seems reasonable for the *BackingStore interfaces given that they already have stop() methods that behave closely to AutoCloseable::close, for example).
2. Add a new variant to Utils::closeQuietly that accepts a possibly-null object and some kind of consumer that can be applied to that object if it's non-null that will close it; I'm imagining calling it would look something like closeQuietly(producer, p -> p.close(duration), "source task producer"); instead of the existing logic for closing a source task's producer, or closeQuietly(offsetStore, OffsetBackingStore::stop, "offset backing store") instead of the existing logic for stopping a source task's offset store.

Thanks. I was actually thinking about something like option #2 but then when I saw, there are changes needed at only a couple of places, I didn't proceed. Do you think we can have a follow up ticket for this?

[C0urante]

Do you think we can have a follow up ticket for this?

Yes, this can definitely be a follow-up. Although given how small this PR has become, it'd be fine to add on here as well.

FWIW, the more I think about it the more appealing option 1 seems. The variant I proposed for Utils::closeQuietly is a little inelegant, and doesn't do anything to actually prevent people from falling into this same trap in the future. It's cleaner and, AFAICT, just as safe to switch things over to implementing AutoCloseable.

Edit: Ugh, no, that doesn't address the case of producers and other objects that have more sophisticated cleanup methods that require parameters. Hmm... I think at the very least we might want to put a warning note in the Javadocs for the existing Utils::closeQuietly method calling out the risks of using it with method references and lambdas. Best case, that's all it takes to prevent another instance of the bug that originally caused this ticket; worst case, it's a decent stopgap until we decide on a more effective solution.

[vamossagar12]

Do you think we can have a follow up ticket for this?

Yes, this can definitely be a follow-up. Although given how small this PR has become, it'd be fine to add on here as well.

FWIW, the more I think about it the more appealing option 1 seems. The variant I proposed for Utils::closeQuietly is a little inelegant, and doesn't do anything to actually prevent people from falling into this same trap in the future. It's cleaner and, AFAICT, just as safe to switch things over to implementing AutoCloseable.

Edit: Ugh, no, that doesn't address the case of producers and other objects that have more sophisticated cleanup methods that require parameters. Hmm... I think at the very least we might want to put a warning note in the Javadocs for the existing Utils::closeQuietly method calling out the risks of using it with method references and lambdas. Best case, that's all it takes to prevent another instance of the bug that originally caused this ticket; worst case, it's a decent stopgap until we decide on a more effective solution.

Thanks @C0urante .. yeah that makes sense.. I added the javadocs to the closeQuietly and it's variants. Plz review.

[vamossagar12]

Thanks for that @C0urante .. You're right, your comment made more sense than mine specially with the example provided. I made the edits. @showuon could you also review whenever you get the chance? Thanks!

[vamossagar12]

@showuon , could you plz review these changes whenever you get the chance?

[vamossagar12]

@showuon could you plz review/merge this? Thanks!

[vamossagar12]

@C0urante i believe now even you might be able to merge this :D

[C0urante]

@vamossagar12 LGTM! And just FYI, I've modified the title of the PR to more-accurately reflect the state it's in now, which should make the commit history clearer.

[vamossagar12]

Thanks @C0urante !