Close socket immediately for fire and forget commands

tylerkaraszewski commented 1 month ago

Details

It seems we don't close the sockets for fire and forget commands until after we finish the commands. Let's close them right after we send the response.

Fixed Issues

https://github.com/Expensify/Expensify/issues/411326

Tests

2024-07-10T22:36:43.812029+00:00 expensidev2004 php-fpm: HQsUhy /api.php admin2@test2.com !web! ?api? [info] Set ASYNC NVP expensify_fixAccountDate
2024-07-10T22:36:43.814120+00:00 expensidev2004 php-fpm: HQsUhy /api.php admin2@test2.com !web! ?api? [info] Bedrock\Client - Starting a request ~~ command: 'SetNameValuePair' clusterName: 'auth' headers: '[authToken: '<REDACTED>' name: 'expensify_fixAccountDate' value: '2024-07-10 22:36:43' Connection: 'forget' priority: '500' logParam: 'admin2@test2.com' urlToNewDot: 'https://dev.new.expensify.com:8082/' shouldReadUsingThreads: '1' shouldHandleOnyxUpdates: '1' clientUpdateID: '-1' maxNumberOfUpdates: '500' requestID: 'HQsUhy' lastIP: '10.2.2.1' writeConsistency: 'ASYNC' timeout: '110000']'
2024-07-10T22:36:43.814936+00:00 expensidev2004 bedrock: xxxxxx (BedrockServer.cpp:2074) _acceptSockets [main] [dbug] Accepting socket from '127.0.0.1:49016' on port '0.0.0.0:4444'
2024-07-10T22:36:43.815010+00:00 expensidev2004 php-fpm: HQsUhy /api.php admin2@test2.com !web! ?api? [info] Closing socket after use
2024-07-10T22:36:43.823305+00:00 expensidev2004 php-fpm: HQsUhy /api.php admin2@test2.com !web! ?api? [dbug] Auth - AuthRequest #2 command='Get' rVL='cardList'  
2024-07-10T22:36:43.823841+00:00 expensidev2004 php-fpm: HQsUhy /api.php admin2@test2.com !web! ?api? [info] Bedrock\Client - Starting a request ~~ command: 'Get' clusterName: 'auth' headers: '[cardID: '' includeAllDeactivated: '1' authToken: '<REDACTED>' returnValueList: 'cardList' idempotent: '1' logParam: 'admin2@test2.com' urlToNewDot: 'https://dev.new.expensify.com:8082/' shouldReadUsingThreads: '1' shouldHandleOnyxUpdates: '1' clientUpdateID: '-1' maxNumberOfUpdates: '500' requestID: 'HQsUhy' lastIP: '10.2.2.1' writeConsistency: 'ASYNC' priority: '500' timeout: '110000']'
2024-07-10T22:36:43.823942+00:00 expensidev2004 php-fpm: HQsUhy /api.php admin2@test2.com !web! ?api? [info] Bedrock\Client - APC fetch host configs ~~ 0: 'auth'
2024-07-10T22:36:43.824018+00:00 expensidev2004 php-fpm: HQsUhy /api.php admin2@test2.com !web! ?api? [info] Bedrock\Client - Possible hosts ~~ nonBlacklistedHosts: '[0: 'auth']'
2024-07-10T22:36:43.824098+00:00 expensidev2004 php-fpm: HQsUhy /api.php admin2@test2.com !web! ?api? [info] Bedrock\Client - Opening new socket ~~ host: 'auth' cluster: 'auth' pid: '1102'

Internal Testing Reminder: when changing bedrock, please compile auth against your new changes

tylerkaraszewski commented 1 month ago

Added test logs.

johnmlee101 commented 1 month ago

reverting this since it created flakey tests here: https://expensify.slack.com/archives/C07J32337/p1720694871685509

tylerkaraszewski commented 1 month ago

I will investigate next week.

iwiznia commented 1 month ago

hmmmm did not read that massive thread, but my guess is: before we were basically waiting for the fire&forget command to finish whenever we did another call to that bedrock instance, with this change we are not, so some tests expect some data to be set, but it is sometimes not because the fire&forget command runs/finishes after the test checks for it.

johnmlee101 commented 1 month ago

Yeah, that's basically the gist. We'll want to make sure our test suite is set beforehand, but the change itself should still be good

Expensify / Bedrock