faye / websocket-driver-ruby

WebSocket protocol handler with pluggable I/O
Other
223 stars 43 forks source link

Pure Ruby masking #48

Closed kbarrette closed 7 years ago

kbarrette commented 8 years ago

I was having some trouble with Windows, bundler, and the native extensions (see also https://github.com/faye/websocket-driver-ruby/issues/12), so here's a pure Ruby version.

/cc @jcoglan

kbarrette commented 8 years ago

Build appears to be failing because it's trying to run the no-longer-present compile rake task.

jcoglan commented 8 years ago

I know that native extensions can pose a problem on Windows. However, I would rather we solve that problem by fixing the native extensions, than by removing them. The native extension in this gem exists because it provides a significant performance improvement over implementing the same logic in Ruby, and I'm unwilling to get rid of it until we've exhausted every possibility for making the native code work with Windows.

kbarrette commented 8 years ago

@jcoglan I thought that might be the reason for the native extensions. Can you show me how to benchmark the various masking implementations?

Also, how do you feel about pluggable masking? I'd really love to have a pure-Ruby approach here, and I'd like to avoid having to switch to another gem or fork this one.

kbarrette commented 8 years ago

Updated with better-performing pure Ruby implementation.

jcoglan commented 7 years ago

It's a long time since I did the work that introduced the native extensions and I've forgotten what tooling I used at the time, and couldn't give you numbers for current ruby versions. However there are various WS benchmarking tools on github.

jcoglan commented 7 years ago

I'm resistant to going down this path for a couple of reasons.

First, increasing the number of implementations we need to maintain increases the build complexity and the chances for inconsistencies. In future, I would like more of this library to be written in C, because it's very well suited to the type of processing required here. Ideally I would like to drop the Java version, and eventually work on JRuby will let us use the C extension there too. Having to maintain multiple versions of each bit of functionality increases my workload.

Second, a pluggable interface is unlikely to work. The set of functionality covered by C code is likely to change over time and it will be hard to define a stable interface where other implementations could be plugged in. Plugins make plenty of sense for composing functionality -- see the extensions support we already have -- but not for swapping out random bits of performance hacks.

In any case, non of this will solve the problem of a gem with C code in it refusing to compile and install on Windows. Do you know of a fallback mechanism that would allow the gem install even if the C code refuses to compile?

jcoglan commented 7 years ago

As a middle ground, I would accept a solution that gets the gem working on your system, that does not delete all the native code from the gem.

kbarrette commented 7 years ago

@jcoglan added an additional fallback as per your "middle ground" suggestion

jcoglan commented 7 years ago

Just to clarify, what's the experience like at installation time? I was never clear whether your issues stopped the gem from installing, or whether it installed fine but failed to load the native code.

kbarrette commented 7 years ago

@jcoglan the installation went fine, but there was a runtime failure, basically exactly like #12

I've been running a forked version of this gem containing my pure-Ruby code with good success, but I'd love to get this fallback version merged and be able to use the main line gem.

jcoglan commented 7 years ago

Does your code work if you remove the clone? The existing masking code doesn't copy the string, it mutates it in place.

kbarrette commented 7 years ago

It should work fine without the clone - updated.

kbarrette commented 7 years ago

@jcoglan also, Travis now thinks this is OK (not sure why it failed the previous build)

jcoglan commented 7 years ago

Thanks a lot for fixing this. I'm now looking at doing a completely native parser, and that will probably result in there being a wholesale switch between a native parser and a pure ruby one, depending on if I can get the interfaces right.

kbarrette commented 7 years ago

@jcoglan thanks for merging! Hopefully you can release a new version soon and I can ditch my forked version of this gem.