Modify

Opened 3 years ago

Last modified 3 years ago

#9603 new defect

Maintainership of the Growl Plugin

Reported by: azurief Owned by: eblot
Priority: normal Component: GrowlPlugin
Severity: normal Keywords: Maintainership
Cc: Trac Release: 0.11

Description

Hello everybody,

I would like to know if this plugin is still being maintained by someone at least. I hacked on it and migrated it to the new GNTP protocol which allows to make it work with the newer versions of Growl and Growl for Windows.

I, at least, would like to commit the code but I also don't mind getting the maintainership of this plugin.

Attachments (0)

Change History (11)

comment:1 Changed 3 years ago by eblot

Not actively maintained, but if you have a patch for the new protocol, I'll be glad to commit it.

However, how the new protocol works? I read a long time ago that the Growl developers wanted to get rid of the UDP protocol, which is IMHO just a huge mistake.

Is GNTP TCP-based, or is it still possible to use UDP?

comment:2 follow-up: Changed 3 years ago by azurief

The new protocol is indeed TCP-based, as the growl developers wanted to have a bi-directive communication.

The UDP protocol has been droped at least by Growl(Mac) and Growl for Windows, which I use. Growl for Windows can only receive notifications from the GNTP protocol but can broadcast them in UDP to older clients.

comment:3 in reply to: ↑ 2 Changed 3 years ago by eblot

Replying to azurief:

The new protocol is indeed TCP-based, as the growl developers wanted to have a bi-directive communication.

Very bad idea(TM)

Ok, so a new configuration option is required to support this new, heavy protocol. Deadlocking the Trac webserver to push a notification will a synonym of major potential issues ahead.

Does your patch support this kind of option?

comment:4 Changed 3 years ago by eblot

s/will/is/

comment:5 follow-up: Changed 3 years ago by azurief

I need to modify the patch to implement a configuration option but my patch does indeed support issues where the clients don't answer to the server sending notifications. If there's a problem sending notifications to one of the clients, it's logged but there's no deadlocking on Trac side.

comment:6 in reply to: ↑ 5 Changed 3 years ago by eblot

Replying to azurief:

I need to modify the patch to implement a configuration option but my patch does indeed support issues where the clients don't answer to the server sending notifications. If there's a problem sending notifications to one of the clients, it's logged but there's no deadlocking on Trac side.

I was referring to the TCP level: you do manage low level TCP connection issues?

comment:7 Changed 3 years ago by azurief

The low level TCP connection is handled by the GNTP python library. This library is referenced on the Growl for Windows website and is actively being maintained.

I know that this plugin was originally a standalone one not relying on any third party library but the new protocol being more complex than the UDP one, I prefer to rely on something used and validated rather than redo the same thing.

comment:8 Changed 3 years ago by eblot

It does not seem that connection issues are not managed at all within this library:

s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
        s.connect((self.hostname, self.port))
        s.send(data.encode('utf8', 'replace'))
        response = gntp.parse_gntp(s.recv(1024))
        s.close()

Timeout management seems to be the default one, etc.
In order words, if the receiving side is accepting the connection and stops, Trac will simply freeze.

It exactly looks the same as the default SMTP notification from Trac, with an even less reliable peer as Growl "server" is a brand new implementation and runs on a less reliable machine than a full blown server most of the time.

I don't mind adding this new protocol to the plugin, as it is a required protocol for the paid version of Growl but:

  • the documentation will require a serious disclaimer,
  • previous protocol should be kept with a version switch (UDP / GNTP) for those who want to be sure that Trac can run w/o deadlocks.

What a strange idea to enforce TCP for a notification subsystem :-(

comment:9 follow-up: Changed 3 years ago by azurief

Ok, indeed, didn't consider this use case...

I will rework my code at the same time to implement a queue. This way notification will handle asynchronously and won't block Trac.

comment:10 in reply to: ↑ 9 Changed 3 years ago by eblot

Replying to azurief:

I will rework my code at the same time to implement a queue. This way notification will handle asynchronously and won't block Trac.

Hmmm. Do not: managing background threads is even more difficult. This is why it has never been implemented for the SMTP notification. Corner cases are even more difficult to manage (shutdown being one of them) - not mentioning that Trac itself can run from multi-processes and/or multithreads.

I guess the best option is to tweak the TCP connection parameters to reduce deadlocks - if it is even possible.

TCP is evil for this kind of task.

comment:11 Changed 3 years ago by azurief

Ok, I looked out a bit and the following looks interesing : Blocking/Unblocking mode for sockets

This might be the workaround needed. Will check out if the GNTP library make use of it.

Add Comment

Modify Ticket

Action
as new .
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.