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

feat: Updates shotgun version to 1.1.0 release #139

Merged

Conversation

supersimple
Copy link
Contributor

@supersimple supersimple commented Nov 7, 2024

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions

Related issues

Provide links to any issues in this repository or elsewhere relating to this pull request.
None found.

Describe the solution you've provided
There was a new version of shotgun released on Oct 24 that has an additional callback for a down message. Upgrading the dependency will support that.

Describe alternatives you've considered
The alternative would be not to update the dependencies

Additional context
The applications I am using this in are getting those messages sent now and ending up in undefined function errors in our error tracking tool. It is not a great consequence, but upgrading the dependency seems like a good practice anyway.

@supersimple supersimple requested a review from a team as a code owner November 7, 2024 16:34
@kinyoklion
Copy link
Member

Hello @supersimple,

Thank you for the contribution. Is there anything specific you are hoping to get from this update, or any specific motivator for the update?

Thank you,
Ryan

@supersimple
Copy link
Contributor Author

Hello @supersimple,

Thank you for the contribution. Is there anything specific you are hoping to get from this update, or any specific motivator for the update?

Thank you, Ryan

Sorry, just updated the description

@kinyoklion
Copy link
Member

Thank you @supersimple,

Ok, So you have upgraded your gun dependency and now gun is sending gun_down in some condition it didn't before? Because the down internal to shotgun is both transitioned to and handled by shotgun, so if you have the new version it would both trigger it and handle it. If you have the old version it wouldn't be triggered and therefore couldn't be unhandled.

I want to make sure that what you are seeing isn't just a bug in the new version of shotgun, and upgrading shotgun will result in other users of this library also experiencing that bug.

For smaller dependencies I audit what changes, the main change to shotgun has been adding a reopen function (which sets some state data used to trigger a re-connect on a gun_down). It did already handle gun_down though, but the handling is a bit different. It did add another handler for a theoretically unexpected case.

Thank you,
Ryan

@supersimple
Copy link
Contributor Author

The trace from my app is:

Elixir.FunctionClauseError no function clause matches 
    /home/runner/.../deps/shotgun/src/shotgun.erl:438 :shotgun.at_rest(:cast, {:gun_down, #PID<0.370.0>, :http, :closed, [#Reference<0.2262378443.396886020.75886>]}, %{async: true, async_mode: :sse, buffer: ":\n:\n:\n:\n:\n", data: "", from: {#PID<0.367.0>, [:alias | #Reference<0.2262378443.396951556.75885>]}, handle_event: #Function<0.82739534/3 in :ldclient_update_stream_server.do_listen/5>, headers: [{"date", "Fri, 01 Nov 2024 18:41:13 GMT"}, {"content-type", "text/event-stream; charset=utf-8"}, {"transfer-encoding", "chunked"}, {"connection", "keep-alive"}, {"accept-ranges", "bytes"}, {"cache-control", "no-cache, no-store, must-revalidate"}, {"etag", "\"452c\""}, {"ld-region", "us-east-1"}, {"strict-transport-security", "max-age=31536000; includeSubDomains"}, {"strict-transport-security", "max-age=31536000; includeSubDomains"}], pending_requests: {[], []}, pid: #PID<0.370.0>, responses: {[], []}, status_code: 200, stream: #Reference<0.2262378443.396886020.75886>})
    gen_statem.erl:1203 :gen_statem.loop_state_callback/11
    proc_lib.erl:226 :proc_lib.init_p_do_apply/3

It looks like this is the change that applies: https://diff.hex.pm/diff/shotgun/1.0.1..1.1.0#87063449--393

@kinyoklion
Copy link
Member

The trace from my app is:

Elixir.FunctionClauseError no function clause matches 
    /home/runner/.../deps/shotgun/src/shotgun.erl:438 :shotgun.at_rest(:cast, {:gun_down, #PID<0.370.0>, :http, :closed, [#Reference<0.2262378443.396886020.75886>]}, %{async: true, async_mode: :sse, buffer: ":\n:\n:\n:\n:\n", data: "", from: {#PID<0.367.0>, [:alias | #Reference<0.2262378443.396951556.75885>]}, handle_event: #Function<0.82739534/3 in :ldclient_update_stream_server.do_listen/5>, headers: [{"date", "Fri, 01 Nov 2024 18:41:13 GMT"}, {"content-type", "text/event-stream; charset=utf-8"}, {"transfer-encoding", "chunked"}, {"connection", "keep-alive"}, {"accept-ranges", "bytes"}, {"cache-control", "no-cache, no-store, must-revalidate"}, {"etag", "\"452c\""}, {"ld-region", "us-east-1"}, {"strict-transport-security", "max-age=31536000; includeSubDomains"}, {"strict-transport-security", "max-age=31536000; includeSubDomains"}], pending_requests: {[], []}, pid: #PID<0.370.0>, responses: {[], []}, status_code: 200, stream: #Reference<0.2262378443.396886020.75886>})
    gen_statem.erl:1203 :gen_statem.loop_state_callback/11
    proc_lib.erl:226 :proc_lib.init_p_do_apply/3

It looks like this is the change that applies: https://diff.hex.pm/diff/shotgun/1.0.1..1.1.0#87063449--393

Yeah, this is what I was referencing here.

It did add another handler for a theoretically unexpected case.

It isn't as unexpected as the comments imply apparently.

Thank you,

@kinyoklion kinyoklion changed the title feat: Updates shotgun version to newest release feat: Updates shotgun version to 1.1.0 release Nov 7, 2024
@kinyoklion kinyoklion merged commit 52dafc3 into launchdarkly:main Nov 7, 2024
6 checks passed
kinyoklion pushed a commit that referenced this pull request Nov 7, 2024
🤖 I have created a release *beep* *boop*
---


##
[3.5.0](v3.4.0...v3.5.0)
(2024-11-07)


### Features

* Updates shotgun version to 1.1.0 release
([#139](#139))
([52dafc3](52dafc3))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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