Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: close the UdpWriter #144

Merged
merged 3 commits into from
Jul 21, 2023
Merged

Conversation

sokil
Copy link
Contributor

@sokil sokil commented Jul 20, 2023

Proposed Changes

If UPD socket was previously closed by \InfluxDB2\UdpWriter::close and new write occured, error will be obtained socket_sendto(): Argument #1 ($socket) has already been closed

    $writeApi = $client->createUdpWriter();
    $writeApi->write("weather,location=Tokio temperature=$temp $seconds");
    $writeApi->close();
    $writeApi->write("weather,location=Tokio temperature=$temp $seconds"); // uses link to closed socket

This is because method close does not unset socket property:

    public function close()
    {
        if (isset($this->socket)) {
            socket_close($this->socket);
        }
    }

And getSocket does not create new connection because old still in socket property:

    protected function getSocket()
    {
        if (empty($this->socket)) {
            $this->socket = socket_create($this->getConfiguredInetVersion(), SOCK_DGRAM, SOL_UDP);
        }
        return $this->socket;
    }

Checklist

  • CHANGELOG.md updated
  • Rebased/mergeable
  • make test completes successfully
  • Commit messages are conventional
  • Sign CLA (if not already signed)

@codecov-commenter
Copy link

codecov-commenter commented Jul 20, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.07 ⚠️

Comparison is base (5ff8234) 74.93% compared to head (123f1dd) 74.86%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #144      +/-   ##
============================================
- Coverage     74.93%   74.86%   -0.07%     
  Complexity      424      424              
============================================
  Files            25       25              
  Lines          1093     1094       +1     
============================================
  Hits            819      819              
- Misses          274      275       +1     
Impacted Files Coverage Δ
src/InfluxDB2/UdpWriter.php 39.47% <0.00%> (-1.07%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@bednar bednar changed the title set socket to null fix: close the UdpWriter Jul 21, 2023
Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sokil thanks for your PR 👍

Can you please satisfy our checklist? After that we will be able to merge this PR:

image

@sokil sokil force-pushed the fix-udp-writer-close branch from 43da760 to 2feb65c Compare July 21, 2023 08:43
@sokil
Copy link
Contributor Author

sokil commented Jul 21, 2023

done

@bednar bednar merged commit a7e2fcd into influxdata:master Jul 21, 2023
@bednar bednar added this to the 3.3.0 milestone Jul 21, 2023
@bednar
Copy link
Contributor

bednar commented Jul 21, 2023

@sokil thanks again for PR 👍

@sokil
Copy link
Contributor Author

sokil commented Jul 21, 2023

Thank you for quick feedback!!

@bednar bednar modified the milestones: 3.3.0, 3.4.0 Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants