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: use unsynced version of `host` attribute to workaround exception #24

Closed

Conversation

Projects
None yet
3 participants
@yaauie
Copy link
Contributor

commented Dec 5, 2017

When a Cinch::User#host is sent, and the underlying attribute is not
populated, Cinch raises a Cinch::Exceptions::SyncedAttributeNotAvailable
exception after timing out waiting for a synchronisation that may never
happen.

Instead, fetch the :host from Cingh::User#data, which will be present
if initially parsed and doesn't rely on Cinch for synchronisation.

Resolves: #9

@yaauie yaauie force-pushed the yaauie:fix/unreliable-host-property branch 2 times, most recently from e050253 to 3d2eb98 Dec 5, 2017

@elasticsearch-bot elasticsearch-bot self-assigned this Dec 5, 2017

@@ -148,7 +148,7 @@ def handle_response (msg, output_queue)
event.set("channel", msg.channel.to_s)
event.set("nick", msg.user.nick)
event.set("server", "#{@host}:#{@port}")
event.set("host", msg.user.host)
event.set("host", msg.user.data[:host]) # use unsynced host because Cinch hangs if unavailable

This comment has been minimized.

Copy link
@andrewvc

andrewvc Dec 6, 2017

What do you mean by unsynced? Is msg.user.host guarded by a mutex, while .data is not?

This comment has been minimized.

Copy link
@yaauie

yaauie Dec 6, 2017

Author Contributor

It's not as straight-forward as a mutex in implementation, but yes; Cinch::User#data is a documented public method that gives access to the underlying data Hash without waiting for synchronisation.

The Cinch::User object is essentially created by parsing the components of each IRC message, and the host attribute is an optional part of the message format; when it's not present, the field is not marked as synchronised, so when we send Cinch::User#host, it waits for the value to get populated -- but doesn't actually kick off anything to populate it -- and then eventually times out and raises an exception.

If the host attribute is present when the message is parsed, it will be populated in the data Hash.

More context below:

I took a brief dive into cinch and found
its model to be rather tangled; the Cinch::User, Cinch::UserList,
Cinch::Bot (which is a Cinch::User?), and Cinch::IRC apparently
attempt to work together to keep user metadata in sync, but to do so they
juggle a variety of explicitly-given and externally-fetched data and it's not
clear whose responsibility it is to mark portions as synced or really what
"synced" even means.

For whatever reason, a message is being processed where the user's host
attribute isn't part of the message, so it never gets marked as synced and
times out waiting for an update that will never happen. [...]

@yaauie in Issue Comment

This comment has been minimized.

Copy link
@andrewvc

andrewvc Dec 6, 2017

@yaauie that makes sense can we add just a bit more color to the comment along the lines of this note?

@andrewvc
Copy link

left a comment

LGTM (pending an improved comment about the synced bit)

fix: use unsynced version of `host` attribute to workaround exception
When a `Cinch::User#host` is sent, and the underlying attribute is not
populated, Cinch raises a `Cinch::Exceptions::SyncedAttributeNotAvailable`
exception after timing out waiting for a synchronisation that may never
happen.

Instead, fetch the `:host` from `Cingh::User#data`, which will be present
if initially parsed and doesn't rely on `Cinch` for synchronisation.

Resolves: #9
Closes: #10

@yaauie yaauie force-pushed the yaauie:fix/unreliable-host-property branch from 3d2eb98 to cdcbd0a Dec 7, 2017

@elasticsearch-bot

This comment has been minimized.

Copy link

commented Dec 7, 2017

Ry Biesemeyer merged this into the following branches!

Branch Commits
master 7a3c013

@yaauie yaauie deleted the yaauie:fix/unreliable-host-property branch Dec 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.