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

Added ability to create custom extended ProtocolProcessors and WebSocket clients #12

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

larskn
Copy link
Contributor

@larskn larskn commented Jun 6, 2014

In some scenarios it is neccesary to create an extended WebSocket and ProtocolProcessor, e.g. to add certain steps to the handshake (for an example see: https://github.com/Atmosphere/WebSocket4Net/tree/AtmosphereFrameworkSupport).
With the current version of WebSocket4Net this requires either modifications to the existing framework code (as seen in the example) or at least modification of the WebSocket4Net assembly (when using partial classes).
I made some modifications so an extended WebSocket and ProtocolProcessor can be created outside the WebSocket4Net assembly and have (protected) access to the required attributes. This is just a proposal, but it would make the creation of custom extension (e.g. for the Atmosphere protocol) a lot easier.

@kerryjiang
Copy link
Owner

I think this change do two things:

  1. pass in custom header items
  2. hook handshake event

for 1), you can pass in custom header items from construction parameter; for 2), I think we can added event or protected method to allow override?

@larskn
Copy link
Contributor Author

larskn commented Jun 10, 2014

This change can do a bit more. Let me explain.

I have to admit I come from a very specific use case.
I'm trying to build a .NET client for the Atmosphere Framework (https://github.com/Atmosphere/atmosphere) like the one I linked to but with a few improvements and an important premise: I don't want to modify exisiting WebSocket4Net code and (if possible) I don't want to rebuild the WebSocket4Net assembly.
The first one would be possible via partial classes, but it would still be required to build these into the WebSocket4Net assembly.

The ideal case for me would be that I could just use WebSocket4Net as a Nuget dependency. But some operations are not possible from outside in the current state.
These operations are required by the atmosphere protocol (https://github.com/Atmosphere/atmosphere/wiki/Understanding-the-Atmosphere-Protocol):

  • Modify headers and/or Url-Query-Parameters (meaning the TargetUri) in VerifyHandshake (Atmosphere sends back a unique tracking id for the client).
  • Modify headers and/or Url-Query-Parameters (meaning the TargetUri) in SendHandshake to include the current timestamp.
  • Send an Atmposphere-specific close request during SendCloseHandshake

My proposal was the most general approach I could think of so others could maybe also benefit from this change.

One more restrictive change that would also work (at least for headers) would be:

  1. Add ability to specify custom ProtocolProcessor (or alternatively a way to hook into the handshake events)
  2. Add the ability to add and remove headers from outside (either make it public or (maybe a little prettier) add an addHeader(String,String)/removeHeader(String,String) method pair)

@kerryjiang
Copy link
Owner

I know your idea.
But I think there might be an easier way.

  1. as I mentioned websocket4net allow you pass in any customized header items
  2. there is already a virtual method named "OnHandshaked()" in the websocket class

Do they satisfy your requirement?

@larskn
Copy link
Contributor Author

larskn commented Jun 12, 2014

Sorry, this does not satisfy my requirements. (and sorry for the late answer)

  1. I have to modify the header list in OnHandshaked(). This can't be done in a separate assembly (even with a virtual method) because CustomHeaderItems is internal.
  2. I also have to do custom actions in SendCloseHandshake (or a similar place) since Atmosphere requires a specific close request.

@larskn
Copy link
Contributor Author

larskn commented Jun 12, 2014

Wait, I just thought of a possible solution for 1): I could maybe pass in a reference to a Header-List and then modify it from outside. I will try that tomorrow and keep you informed.

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.

2 participants