davsclaus commented on code in PR #25907: URL: https://github.com/apache/camel/pull/25907#discussion_r3889977012
########## components/camel-mail/src/test/java/org/apache/camel/component/mail/MailConsumerFolderNullCatchTest.java: ########## @@ -0,0 +1,105 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.camel.component.mail; + +import java.lang.reflect.Field; + +import jakarta.mail.Folder; +import jakarta.mail.MessagingException; +import jakarta.mail.Session; +import jakarta.mail.Store; + +import org.apache.camel.CamelContext; +import org.apache.camel.ExtendedCamelContext; +import org.apache.camel.Processor; +import org.apache.camel.component.mail.Mailbox.Protocol; +import org.apache.camel.spi.ExchangeFactory; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Verifies that {@link MailConsumer#poll()} does not throw {@link NullPointerException} in the {@code finally} catch + * block when {@code folder.close()} throws and the folder field is null at the time the catch block runs. + * + * <p> + * Before the fix, the catch block logged {@code folder.getName()} without a null check — if a concurrent + * {@code disconnect()} nulled the field between the {@code if (folder != null)} guard and the catch body, an NPE would + * be thrown instead of the intended debug log. CAMEL-24565. Review Comment: Wrong issue number: this cites `CAMEL-24565`, which is an unrelated dev-console/OpenAPI schema ticket. Should be `CAMEL-24567` (the ticket this PR actually fixes). ########## components/camel-mail/src/main/java/org/apache/camel/component/mail/MailConsumer.java: ########## @@ -190,7 +190,8 @@ protected int poll() throws Exception { } } catch (Exception e) { // some mail servers will lock the folder so we ignore in this case (CAMEL-1263) - LOG.debug("Could not close mailbox folder: {}. This exception is ignored.", folder.getName(), e); + LOG.debug("Could not close mailbox folder: {}. This exception is ignored.", + folder != null ? folder.getName() : "null", e); Review Comment: The premise for this guard — "a concurrent `disconnect()` has nulled the `folder` field between the `if`-guard and the catch body" — doesn't correspond to any real path in this class. Every assignment of `folder = null` (`disconnect()`, `ensureIsConnected()`) happens synchronously inside `poll()` itself, and `doStop()` — the one method that does run concurrently with an in-flight `poll()` — never nulls the field, it only calls `close()` on it. See the review summary for the full trace. The null-check itself is harmless to keep, but the PR/JIRA description should not claim this fixes an observed race. ########## components/camel-mail/src/test/java/org/apache/camel/component/mail/MailConsumerFolderNullCatchTest.java: ########## @@ -0,0 +1,105 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.camel.component.mail; + +import java.lang.reflect.Field; + +import jakarta.mail.Folder; +import jakarta.mail.MessagingException; +import jakarta.mail.Session; +import jakarta.mail.Store; + +import org.apache.camel.CamelContext; +import org.apache.camel.ExtendedCamelContext; +import org.apache.camel.Processor; +import org.apache.camel.component.mail.Mailbox.Protocol; +import org.apache.camel.spi.ExchangeFactory; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Verifies that {@link MailConsumer#poll()} does not throw {@link NullPointerException} in the {@code finally} catch + * block when {@code folder.close()} throws and the folder field is null at the time the catch block runs. + * + * <p> + * Before the fix, the catch block logged {@code folder.getName()} without a null check — if a concurrent + * {@code disconnect()} nulled the field between the {@code if (folder != null)} guard and the catch body, an NPE would + * be thrown instead of the intended debug log. CAMEL-24565. + */ +class MailConsumerFolderNullCatchTest { + + @Test + void testFolderCloseThrowsAndFolderBecomesNullDoesNotNPE() throws Exception { + JavaMailSender sender = mock(JavaMailSender.class); + Processor processor = mock(Processor.class); + CamelContext camelContext = mock(CamelContext.class); + ExtendedCamelContext ecc = mock(ExtendedCamelContext.class); + ExchangeFactory ef = mock(ExchangeFactory.class); + Session session = Session.getInstance(Mailbox.getSessionProperties(Protocol.imap)); + + when(sender.getSession()).thenReturn(session); + when(camelContext.getCamelContextExtension()).thenReturn(ecc); + when(ecc.getExchangeFactory()).thenReturn(ef); + when(ef.newExchangeFactory(any())).thenReturn(ef); + + MailEndpoint endpoint = new MailEndpoint(); + endpoint.setCamelContext(camelContext); + MailConfiguration config = new MailConfiguration(); + config.configureProtocol(Protocol.imap.name()); + config.setPort(Mailbox.getPort(Protocol.imap)); + config.setFolderName("INBOX"); + config.setCloseFolder(true); // ensures the finally close path runs + endpoint.setConfiguration(config); + + MailConsumer consumer = new MailConsumer(endpoint, processor, sender); + + Field folderField = MailConsumer.class.getDeclaredField("folder"); + folderField.setAccessible(true); + Field storeField = MailConsumer.class.getDeclaredField("store"); + storeField.setAccessible(true); + + // set up a folder that: + // 1. isOpen() returns true — so close() will be called + // 2. getMessageCount() returns 0 — so poll returns immediately after + // 3. close() throws MessagingException — triggers the catch block + // AND sets folder field to null to simulate concurrent disconnect() + Folder folder = mock(Folder.class); + when(folder.isOpen()).thenReturn(true); + when(folder.getMessageCount()).thenReturn(0); + doAnswer(inv -> { + // simulate concurrent disconnect() nulling the field while close() runs + folderField.set(consumer, null); + throw new MessagingException("server locked the folder"); + }).when(folder).close(anyBoolean()); + + Store store = mock(Store.class); + when(store.isConnected()).thenReturn(true); + + folderField.set(consumer, folder); + storeField.set(consumer, store); + + // must not throw NullPointerException in the catch block — CAMEL-24565 Review Comment: Same wrong issue number here — `CAMEL-24565` should be `CAMEL-24567`. Also worth noting: this test doesn't reproduce a real interleaving — `folderField.set(consumer, null)` is invoked by hand inside the mocked `close()`, so it proves the null-guard works, not that the guarded scenario is reachable in production. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
